[otlp] Add foundation for signal-specific envvars#5429
Conversation
CodeBlanch
added
the
pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5429 +/- ##
==========================================
+ Coverage 83.38% 84.78% +1.39%
==========================================
Files 297 282 -15
Lines 12531 12170 -361
==========================================
- Hits 10449 10318 -131
+ Misses 2082 1852 -230
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| OtlpSpecConfigDefinitions.TracesHeadersEnvVarName, | ||
| OtlpSpecConfigDefinitions.TracesTimeoutEnvVarName); | ||
| } | ||
| else |
There was a problem hiding this comment.
Remove this code if we don't expect it to called?
There was a problem hiding this comment.
It is a defensive thing. This code is here to alert a future dev who might add a new definition to OtlpExporterOptionsConfigurationType that they need to update this code in order for things to work.
| var options = new OtlpExporterOptions(); | ||
|
|
||
| Assert.Equal(new Uri("http://localhost:4317"), options.Endpoint); | ||
| Assert.Equal(new Uri(OtlpExporterOptions.DefaultGrpcEndpoint), options.Endpoint); |
There was a problem hiding this comment.
I think it's better to use the hardcoded string "http://localhost:4317" for the test instead of relying on the constant defined in src file otherwise the test would never complain even if the constant defined in the src file accidentally gets changed.
There was a problem hiding this comment.
We've debated this before 🤣 I subscribe to the DRY/single source of truth principle. If a dev wants to change the value, it should be defined in a single spot. If a dev changes the constant sure the tests might break. Or that dev could just update the tests too. Either case it is going to have to pass code review. Have we encountered issues with people trying to change our constants to be incorrect?
There was a problem hiding this comment.
Either case it is going to have to pass code review. Have we encountered issues with people trying to change our constants to be incorrect?
I feel, when possible, having unit tests that signal someone that they messed up is better. It's much quicker than the issue being caught in a PR review. It also slightly reduces the burden on the reviewer.
We've debated this before 🤣
Haha yes. I remember now. I'll leave this up to you. It's kind of similar to why we would want to keep this check. We don't expect anyone to add another signal type and any future dev who tries to add it should be informed not to do so in the code review 😀
There was a problem hiding this comment.
Either case it is going to have to pass code review. Have we encountered issues with people trying to change our constants to be incorrect?
I feel, when possible, having unit tests that signal someone that they messed up is better. It's much quicker than the issue being caught in a PR review. It also slightly reduces the burden on the reviewer.
+1, I don't have super strong opinion here, but my preference is: if there is a behavior change in the production code, it should be caught by unit test. In other words, if a dev wants to make a behavior change, both production code and unit test code need to be updated.
| Environment.SetEnvironmentVariable(OtlpExporterSpecEnvVarKeyDefinitions.DefaultProtocolEnvVarName, "invalid"); | ||
| var values = new Dictionary<string, string>() | ||
| { | ||
| ["InvalidEndpoint"] = "invalid", |
There was a problem hiding this comment.
Why are we testing the behavior for invalid keys? This test was meant to test the behavior with invalid values, right? We would never provide invalid key names as parameters for ApplyConfigurationUsingSpecificationEnvVars anyway.
There was a problem hiding this comment.
Sorry those names were kind of misleading. There is nothing invalid about the names, only the values. I just tweaked the names so it should be less confusing now.
4 participants
[Originally part of #5400]
Changes
OtlpExporterOptionsusing signal-specific spec envvars. Note: This is NOT currently utilized meaningAddOtlpExporterextension does NOT gain the ability to use these as of this PR.Merge requirement checklist