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 Marshaling implementations for exporters #2578

Merged
merged 6 commits into from Feb 25, 2022

Conversation

MadVikingGod
Copy link
Contributor

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.

@codecov
Copy link

codecov bot commented Feb 2, 2022

Codecov Report

Merging #2578 (ffc92f0) into main (76bff3f) will decrease coverage by 0.1%.
The diff coverage is 0.0%.

Impacted file tree graph

@@           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     
Impacted Files Coverage Δ
exporters/jaeger/jaeger.go 90.3% <0.0%> (-2.4%) ⬇️
exporters/otlp/otlptrace/exporter.go 0.0% <0.0%> (ø)
exporters/otlp/otlptrace/otlptracegrpc/client.go 92.1% <0.0%> (-5.6%) ⬇️
exporters/otlp/otlptrace/otlptracehttp/client.go 76.9% <0.0%> (-4.9%) ⬇️
exporters/zipkin/zipkin.go 66.6% <0.0%> (-5.7%) ⬇️
sdk/trace/simple_span_processor.go 74.5% <ø> (ø)
sdk/metric/sdk.go 81.5% <0.0%> (+1.4%) ⬆️
sdk/metric/refcount_mapped.go 100.0% <0.0%> (+20.0%) ⬆️

@dmathieu
Copy link
Member

dmathieu commented Feb 3, 2022

Should there be tests?

@MadVikingGod
Copy link
Contributor Author

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.

@dmathieu
Copy link
Member

dmathieu commented Feb 8, 2022

Not much I suppose, but checking that the method actually runs, and has the expected value?

@MadVikingGod
Copy link
Contributor Author

Like I stated before, testing would not be testing our code but testing the code of logr. So any test that I would write will test only our implementation, which is the recipe for a stale test.

@MrAlias MrAlias added this to the Release v1.5.0 milestone Feb 24, 2022
@MrAlias
Copy link
Contributor

MrAlias commented Feb 25, 2022

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.

@MrAlias MrAlias merged commit a1fff3c into open-telemetry:main Feb 25, 2022
@MrAlias MrAlias mentioned this pull request Mar 15, 2022
@MadVikingGod MadVikingGod deleted the exporters-marshaling branch February 21, 2023 19:57
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.

None yet

5 participants