-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add Marshaling implementations for exporters #2578
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 Marshaling implementations for exporters #2578
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2578 +/- ##
=======================================
- Coverage 76.2% 76.0% -0.2%
=======================================
Files 173 173
Lines 12253 12293 +40
=======================================
+ Hits 9342 9349 +7
- Misses 2664 2699 +35
+ Partials 247 245 -2
|
Should there be tests? |
What behavior would you expect to test? The marshaling logic is handled by the logr. This just allows for the logs produced to be more detailed. |
Not much I suppose, but checking that the method actually runs, and has the expected value? |
Like I stated before, testing would not be testing our code but testing the code of |
From the SIG meeting yesterday there was a consensus that we likely do not want to test these additions. Doing so would be "change indicators" and likely contain more logic to validate than these additions. That said, if someone wanted to propose changes after this is merged that test this output and didn't suffer from the issues above, we would be interested in reviewing them. There is a desire to not hold up these changes being merged because of this lack of associated tests. |
This change makes it so that the TracerProviders log messages will have the kind of exporter when logged.
There are differences between different exporters because each has different configurations and different levels of information that are accessible when the exporter will be logged.