-
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
454f7c8
d61dc99
210bfed
8972d9d
3933f30
7895f9e
2734a39
ed6a109
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
using System.Diagnostics.CodeAnalysis; | ||
using System.Globalization; | ||
using System.Runtime.CompilerServices; | ||
using System.Text.RegularExpressions; | ||
|
||
#pragma warning disable SA1402 // File may only contain a single type | ||
#pragma warning disable SA1403 // File may only contain a single namespace | ||
|
@@ -46,8 +47,21 @@ namespace OpenTelemetry.Internal | |
/// <summary> | ||
/// Methods for guarding against exception throwing values. | ||
/// </summary> | ||
internal static class Guard | ||
internal static partial class Guard | ||
{ | ||
// 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 NET7_0_OR_GREATER | ||
Kielek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
[GeneratedRegex(@"^[a-z][a-z0-9-._/]{0,254}$", RegexOptions.IgnoreCase)] | ||
public static partial Regex InstrumentNameRegex(); | ||
Kielek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
#else | ||
public static Regex InstrumentNameRegex { get; set; } = new( | ||
@"^[a-z][a-z0-9-._/]{0,254}$", RegexOptions.IgnoreCase | RegexOptions.Compiled); | ||
#endif | ||
|
||
/// <summary> | ||
/// Throw an exception if the value is null. | ||
/// </summary> | ||
|
@@ -178,6 +192,84 @@ public static T ThrowIfNotOfType<T>([NotNull] object? value, [CallerArgumentExpr | |
return result; | ||
} | ||
|
||
/// <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([NotNull] string? viewName, [CallerArgumentExpression(nameof(viewName))] string? paramName = null) | ||
#pragma warning disable CS8777 // Parameter must have a non-null value when exiting. | ||
martincostello marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
{ | ||
if (!IsValidInstrumentName(viewName)) | ||
{ | ||
throw new ArgumentException($"Custom view name {viewName} is invalid.", paramName); | ||
} | ||
} | ||
#pragma warning restore CS8777 // Parameter must have a non-null value when exiting. | ||
|
||
/// <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(string? instrumentName) | ||
{ | ||
if (string.IsNullOrWhiteSpace(instrumentName)) | ||
{ | ||
return false; | ||
} | ||
#if NET7_0_OR_GREATER | ||
Kielek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
return InstrumentNameRegex().IsMatch(instrumentName); | ||
#else | ||
return InstrumentNameRegex.IsMatch(instrumentName); | ||
#endif | ||
} | ||
|
||
/// <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; | ||
} | ||
#if NET7_0_OR_GREATER | ||
hannahhaering marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
return InstrumentNameRegex().IsMatch(customViewName); | ||
#else | ||
return InstrumentNameRegex.IsMatch(customViewName); | ||
#endif | ||
} | ||
|
||
[DebuggerHidden] | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
private static void Range<T>(T value, string? paramName, T min, T max, string? minName, string? maxName, string? message) | ||
|
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.