Skip to content
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

Add validation to instrument and view name #2470

Conversation

joaopgrassi
Copy link
Member

@joaopgrassi joaopgrassi commented Oct 8, 2021

Changes

This PR adds validation to the instrument names as described in the Instrument spec.

Summary of changes:

  • Instruments with invalid names are ignored and an event is logged
  • Views added with a name also are validated in the same way with some particularities:
    • Overloads of AddView that accepts the name or MetricStreamConfiguration fail-fast if the name provided is invalid
    • Overloads where the Func is specified cannot be validated right away. They are only validated on instrument publish, and if invalid, are ignored with a log.
  • Added new events in OpenTelemetrySdkEventSource to log the above cases
  • Tests

Closes #2424

For significant contributions please make sure you have completed the following items:

@joaopgrassi joaopgrassi requested a review from a team October 8, 2021 09:47
@joaopgrassi joaopgrassi changed the title WIP: Initial validation of instrument name Add validation to instrument and view name Oct 8, 2021
@joaopgrassi joaopgrassi force-pushed the instrumentname-validation branch 3 times, most recently from 784450d to a456557 Compare October 8, 2021 10:15
@codecov
Copy link

codecov bot commented Oct 8, 2021

Codecov Report

Merging #2470 (74ae44d) into main (d9b2ea4) will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2470      +/-   ##
==========================================
+ Coverage   80.73%   80.83%   +0.10%     
==========================================
  Files         254      254              
  Lines        8516     8553      +37     
==========================================
+ Hits         6875     6914      +39     
+ Misses       1641     1639       -2     
Impacted Files Coverage Δ
...elemetry/Metrics/MeterProviderBuilderExtensions.cs 76.92% <100.00%> (+6.92%) ⬆️
...c/OpenTelemetry/Metrics/MeterProviderBuilderSdk.cs 100.00% <100.00%> (ø)
src/OpenTelemetry/Metrics/MeterProviderSdk.cs 93.25% <100.00%> (+0.63%) ⬆️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 97.65% <0.00%> (+0.78%) ⬆️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 74.28% <0.00%> (+0.95%) ⬆️

@joaopgrassi joaopgrassi force-pushed the instrumentname-validation branch from af86f5e to e0fd3ba Compare October 11, 2021 10:43
@joaopgrassi
Copy link
Member Author

I had some conflicts after #2459 but rebased and solved them now.

@joaopgrassi joaopgrassi force-pushed the instrumentname-validation branch from aca9143 to e3ea4f0 Compare October 15, 2021 07:10
@joaopgrassi
Copy link
Member Author

joaopgrassi commented Nov 2, 2021

Rebased and fixed new conflicts. Ready to merge 😊

@cijothomas cijothomas merged commit 1d16b0a into open-telemetry:main Nov 2, 2021
@joaopgrassi joaopgrassi deleted the instrumentname-validation branch November 10, 2021 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instrument name validation
6 participants