[chore][service/telemetry] Fill out valid level values and match with definition #12287
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #12287 +/- ##
==========================================
+ Coverage 91.31% 91.34% +0.03%
==========================================
Files 465 466 +1
Lines 25586 25714 +128
==========================================
+ Hits 23363 23489 +126
- Misses 1806 1808 +2
Partials 417 417 ☔ View full report in Codecov by Sentry. |
| // - "basic" is the recommended and covers the basics of the service telemetry. | ||
| // - "None" indicates that no tracing data should be collected. | ||
| // - "Basic" is the recommended and covers the basics of the service telemetry. | ||
| // - "Normal" adds some other indicators on top of basic. |
There was a problem hiding this comment.
Basic and Normal aren't documented here because their behaviour is no different from basic for traces.
There was a problem hiding this comment.
Is there anything currently enforcing the fact that Basic, Normal, and Detailed are all the same at this point?
Out of ignorance, my assumption is that at any point a change could be introduced that makes them different, so we may as well include valid values. Maybe we can add a note that they're currently the same thing? I just lean towards documenting valid values.
| // - "None" indicates that no telemetry data should be collected. | ||
| // - "Basic" is the recommended and covers the basics of the service telemetry. | ||
| // - "Normal" adds some other indicators on top of basic. | ||
| // - "Detailed" adds dimensions and views to the previous levels. |
There was a problem hiding this comment.
Don't know if this means any documentation/examples should also be updated or not.
There was a problem hiding this comment.
I can update documentation and examples. It technically doesn't make a difference, but I figure the closer we are to the defined constant, the less likely we are to run into problems.
There was a problem hiding this comment.
I looked into it a bit more, I think updating documentation examples may be a bit more disconcerting to users than helpful. With the widespread use of these constants in other components (such as the debug exporter), I think it's not worth it. There are also tests to ensure case doesn't impact results, so it should be safe to keep as-is for now.
I can revert this change or leave as-is, let me know if you have a preference.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Description
This updates the trace and metric configs for service telemetry to have all config options commented, along with the exact case of the defined string constants of each level.
Note: The case of
Leveldoesn't matter in practice as they're all compared instrings.ToLowercalls, but I figure exact matching is best here.