Improve instrument name validation and log messages #6457
Conversation
github-actions
bot
added
the
pkg:OpenTelemetry
…to argument exceptions
Co-authored-by: Martin Costello <[email protected]>
Co-authored-by: Martin Costello <[email protected]>
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
github-actions
bot
added
the
Stale
src/Shared/Guard.cs
Outdated
| // 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. |
There was a problem hiding this comment.
It seems to be broking #4136.
In general, it is not against OTel promises, but it might be still useful for @dpk83.
@dpk83, what is your perspective on changes?
@CodeBlanch, FYI as you introduced this functionality.
There was a problem hiding this comment.
There was a problem hiding this comment.
Should I make the changes in the contrib repo or is there anything else left to do?
github-actions
bot
removed
the
Stale
src/Shared/Guard.cs
Outdated
| internal static class Guard | ||
| internal static partial class Guard | ||
| { | ||
| // Note: We don't use static readonly here because some customers |
There was a problem hiding this comment.
One more thing, for now, new logic seems to be related only to metrics. Maybe you could put all this stuff to separate class under src/OpenTelemetry/Metrics/MetricsGuard.cs (better name more than weslcomed)?
Do you think that we need some new unit tests?
There was a problem hiding this comment.
I'm currently working on this.
There was a problem hiding this comment.
On the other hand, this doesn't need to be a regular expression. Length check and IndexOfAny would do the job.
…lidation-and-log-messages
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6457 +/- ##
=======================================
Coverage 86.63% 86.64%
=======================================
Files 258 259 +1
Lines 11888 11887 -1
=======================================
Hits 10299 10299
+ Misses 1589 1588 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| * Moved the `InstrumentNameRegex` from `MeterProviderBuilderSdk` to `Guard`. | ||
| ([#6457](https://github.com/open-telemetry/opentelemetry-dotnet/pull/6457)) | ||
|
|
There was a problem hiding this comment.
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.
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.
Ah OK - I didn't realise it was public.
There was a problem hiding this comment.
It wasn't . It was known internal feature used by reflection. Mostly by MS teams.
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
github-actions
bot
added
the
Stale
Kielek
removed
the
Stale
Fixes #2532
Changes
I moved the validation of view name and instrument name into Guard helpers. I improved the log message, specifying the incorrect metric name, as suggested in the issue.
The code has changed quite a bit since this issue was opened, so I'm not entirely sure whether my approach matches the original intent.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes