-
Notifications
You must be signed in to change notification settings - Fork 847
Improve instrument name validation and log messages #6457
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
hannahhaering
wants to merge
8
commits into
open-telemetry:main
Choose a base branch
from
hannahhaering:improve-instrument-name-validation-and-log-messages
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
454f7c8
Improve instrument name validation and log messages
hannahhaering d61dc99
use RegEx source generator for net7_0_or_greater; add parameter name …
hannahhaering 210bfed
Update src/OpenTelemetry/CHANGELOG.md
hannahhaering 8972d9d
Update src/OpenTelemetry/CHANGELOG.md
hannahhaering 3933f30
improved access for Regex
hannahhaering 7895f9e
Merge remote-tracking branch 'origin' into improve-instrument-name-va…
hannahhaering 2734a39
added metric guard and unit tests
hannahhaering ed6a109
Merge branch 'main' into improve-instrument-name-validation-and-log-m…
hannahhaering File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
// Copyright The OpenTelemetry Authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
using System.Diagnostics; | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.Runtime.CompilerServices; | ||
using System.Text.RegularExpressions; | ||
|
||
namespace OpenTelemetry.Metrics; | ||
|
||
/// <summary> | ||
/// Methods for guarding against exception throwing values in Metrics. | ||
/// </summary> | ||
internal partial class MetricGuard | ||
{ | ||
// Note: We don't use static readonly here because some customers | ||
// replace this using reflection which is not allowed on initonly static | ||
// fields. See: https://github.com/dotnet/runtime/issues/11571. | ||
// Customers: This is not guaranteed to work forever. We may change this | ||
// mechanism in the future do this at your own risk. | ||
#if NET | ||
[GeneratedRegex(@"^[a-z][a-z0-9-._/]{0,254}$", RegexOptions.IgnoreCase)] | ||
public static partial Regex InstrumentNameRegex(); | ||
#else | ||
private static readonly Regex InstrumentNameRegexField = new( | ||
@"^[a-z][a-z0-9-._/]{0,254}$", RegexOptions.IgnoreCase | RegexOptions.Compiled); | ||
|
||
public static Regex InstrumentNameRegex() => InstrumentNameRegexField; | ||
#endif | ||
|
||
/// <summary> | ||
/// Throws an exception if the given view name is invalid according to the specification. | ||
/// Null is valid because the instrument name will be used as the view name. | ||
/// </summary> | ||
/// <remarks>See specification: <see href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument"/>.</remarks> | ||
/// <param name="viewName">The view name.</param> | ||
/// <param name="paramName">The parameter name to use in the thrown exception.</param> | ||
[DebuggerHidden] | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
public static void ThrowIfInvalidViewName(string? viewName, [CallerArgumentExpression(nameof(viewName))] string? paramName = null) | ||
{ | ||
if (!IsValidViewName(viewName)) | ||
{ | ||
throw new ArgumentException($"View name {viewName} is invalid.", paramName); | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Throws an exception if the given custom view name is invalid according to the specification. | ||
/// </summary> | ||
/// <remarks>See specification: <see href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument"/>.</remarks> | ||
/// <param name="viewName">The view name.</param> | ||
/// <param name="paramName">The parameter name to use in the thrown exception.</param> | ||
[DebuggerHidden] | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
public static void ThrowIfInvalidCustomViewName(string? viewName, [CallerArgumentExpression(nameof(viewName))] string? paramName = null) | ||
{ | ||
if (!IsValidInstrumentName(viewName)) | ||
{ | ||
throw new ArgumentException($"Custom view name {viewName} is invalid.", paramName); | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Returns whether the given instrument name is valid according to the specification. | ||
/// </summary> | ||
/// <remarks>See specification: <see href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument"/>.</remarks> | ||
/// <param name="instrumentName">The instrument name.</param> | ||
/// <returns>Boolean indicating if the instrument is valid.</returns> | ||
[DebuggerHidden] | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
public static bool IsValidInstrumentName([NotNullWhen(true)] string? instrumentName) | ||
{ | ||
if (string.IsNullOrWhiteSpace(instrumentName)) | ||
{ | ||
return false; | ||
} | ||
|
||
return InstrumentNameRegex().IsMatch(instrumentName); | ||
} | ||
|
||
/// <summary> | ||
/// Returns whether the given custom view name is valid according to the specification. | ||
/// </summary> | ||
/// <remarks>See specification: <see href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument"/>.</remarks> | ||
/// <param name="customViewName">The view name.</param> | ||
/// <returns>Boolean indicating if the instrument is valid.</returns> | ||
[DebuggerHidden] | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
private static bool IsValidViewName(string? customViewName) | ||
{ | ||
// Only validate the view name in case it's not null. In case it's null, the view name will be the instrument name as per the spec. | ||
if (customViewName == null) | ||
{ | ||
return true; | ||
} | ||
|
||
return InstrumentNameRegex().IsMatch(customViewName); | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
// Copyright The OpenTelemetry Authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
using OpenTelemetry.Metrics; | ||
using OpenTelemetry.Metrics.Tests; | ||
using Xunit; | ||
|
||
namespace OpenTelemetry.Tests.Metrics; | ||
|
||
public class MetricGuardTests | ||
{ | ||
[Theory] | ||
[MemberData(nameof(MetricTestData.InvalidInstrumentNames), MemberType = typeof(MetricTestData))] | ||
[InlineData(null)] | ||
public void IsValidInstrumentName_ReturnsFalse_ForInvalidNames(string? instrumentName) | ||
{ | ||
Assert.False(MetricGuard.IsValidInstrumentName(instrumentName)); | ||
} | ||
|
||
[Theory] | ||
[MemberData(nameof(MetricTestData.ValidInstrumentNames), MemberType = typeof(MetricTestData))] | ||
public void IsValidInstrumentName_ReturnsTrue_ForValidNames(string instrumentName) | ||
{ | ||
Assert.True(MetricGuard.IsValidInstrumentName(instrumentName)); | ||
} | ||
|
||
[Theory] | ||
[MemberData(nameof(MetricTestData.InvalidInstrumentNames), MemberType = typeof(MetricTestData))] | ||
public void ThrowIfInvalidViewName_ThrowsOnInvalid(string? viewName) | ||
{ | ||
var ex = Assert.Throws<ArgumentException>(() => | ||
MetricGuard.ThrowIfInvalidViewName(viewName)); | ||
|
||
Assert.Contains($"View name {viewName} is invalid.", ex.Message, StringComparison.Ordinal); | ||
} | ||
|
||
[Theory] | ||
[MemberData(nameof(MetricTestData.ValidInstrumentNames), MemberType = typeof(MetricTestData))] | ||
[InlineData(null)] // null is valid because the instrument name will be used as the view name. | ||
public void ThrowIfInvalidViewName_DoesNotThrowForValid(string? viewName) | ||
{ | ||
var ex = Record.Exception(() => | ||
MetricGuard.ThrowIfInvalidViewName(viewName)); | ||
|
||
Assert.Null(ex); | ||
} | ||
|
||
[Theory] | ||
[MemberData(nameof(MetricTestData.InvalidInstrumentNames), MemberType = typeof(MetricTestData))] | ||
[InlineData(null)] // null is invalid for custom view names. | ||
public void ThrowIfInvalidCustomViewName_ThrowsOnInvalid(string? customViewName) | ||
{ | ||
var ex = Assert.Throws<ArgumentException>(() => | ||
MetricGuard.ThrowIfInvalidCustomViewName(customViewName)); | ||
|
||
Assert.Contains($"Custom view name {customViewName} is invalid.", ex.Message, StringComparison.Ordinal); | ||
} | ||
|
||
[Theory] | ||
[MemberData(nameof(MetricTestData.ValidInstrumentNames), MemberType = typeof(MetricTestData))] | ||
public void ThrowIfInvalidCustomViewName_DoesNotThrowForValid(string? customViewName) | ||
{ | ||
var ex = Record.Exception(() => | ||
MetricGuard.ThrowIfInvalidCustomViewName(customViewName)); | ||
|
||
Assert.Null(ex); | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like an internal implementation detail - all users need to know is what the external-facing behaviour change is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I added it to the changelog because the regex is public and someone may change it using reflection:
#6457 (comment)
I can also omit this in the changelog, if it is not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah OK - I didn't realise it was public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't . It was known internal feature used by reflection. Mostly by MS teams.