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 auto-configure support for logging-otlp #4879

Merged
merged 7 commits into from
Nov 28, 2022

Conversation

ben-roling
Copy link
Contributor

As I was starting to play around with opentelemetry I'd tried out the "logging" exporter for some testing/debugging and noticed it didn't quite log all the information I was looking for. Then, I noticed there also existed the classes for "logging-oltp", but no means to auto-configure usage. I tried out these changes and it did the trick.

I'm not sure if omission of auto-configure support for this format might have been intentional?

Anyway, I would appreciate any feedback. Thanks!

@ben-roling ben-roling requested a review from a team October 21, 2022 14:48
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 21, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: ben-roling / name: Ben Roling (918faf7)

@codecov
Copy link

codecov bot commented Oct 21, 2022

Codecov Report

Base: 91.18% // Head: 91.13% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (318f2d6) compared to base (cbd629c).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 318f2d6 differs from pull request most recent head 8b3c3e6. Consider uploading reports for the commit 8b3c3e6 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4879      +/-   ##
============================================
- Coverage     91.18%   91.13%   -0.05%     
+ Complexity     4835     4826       -9     
============================================
  Files           548      545       -3     
  Lines         14361    14339      -22     
  Branches       1371     1369       -2     
============================================
- Hits          13095    13068      -27     
- Misses          876      879       +3     
- Partials        390      392       +2     
Impacted Files Coverage Δ
.../autoconfigure/LogRecordExporterConfiguration.java 93.65% <100.00%> (-4.82%) ⬇️
...sdk/autoconfigure/MetricExporterConfiguration.java 89.85% <100.00%> (-7.16%) ⬇️
...y/sdk/autoconfigure/SpanExporterConfiguration.java 94.87% <100.00%> (-3.88%) ⬇️
...entelemetry/sdk/logs/export/LogRecordExporter.java 91.66% <0.00%> (-8.34%) ⬇️
...metry/sdk/logs/export/BatchLogRecordProcessor.java 88.97% <0.00%> (-0.40%) ⬇️
...etry/sdk/logs/export/SimpleLogRecordProcessor.java 88.57% <0.00%> (-0.32%) ⬇️
...in/java/io/opentelemetry/sdk/OpenTelemetrySdk.java 96.42% <0.00%> (ø)
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ben-roling ben-roling force-pushed the autoconfigure_logging_otlp branch from b8fc7c5 to 918faf7 Compare October 25, 2022 00:18
@jkwatson
Copy link
Contributor

jkwatson commented Nov 2, 2022

please include tests to cover the new code

"io.opentelemetry.exporter.logging.otlp.OtlpJsonLoggingLogRecordExporter",
"OTLP JSON Logging Log Exporter",
"opentelemetry-exporter-logging-otlp");
return OtlpJsonLoggingLogRecordExporter.create();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about providing a ConfigurableLogRecordExporterProvider and letting the autoconfigure discover the exporter automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could give a shot if it's desired. Initially I was just following the code pattern used for the "logging" exporter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been thinking this may be a good idea to do for all the exporters from a separation of concerns perspective. If each exporter artifact contains a Configurable{Signal}ExporterProvider, then the code to interpret ConfigProperties becomes distributed into each artifacts, reducing the complexity of autoconfigure, especially around testing which has become cumbersome.

What do you think @jkwatson?

BTW, for the purposes of this PR I do think we should continue the current pattern. If we agree on implementing providers, we can open a separate issue to track that work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then the code to interpret ConfigProperties becomes distributed into each artifacts, reducing the complexity of autoconfigure, especially around testing which has become cumbersome.

this sounds great, and could help guide users into good practices when they are trying to extend autoconfigure with their own items (e.g. exporters)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, for the purposes of this PR I do think we should continue the current pattern. If we agree on implementing providers, we can open a separate issue to track that work.

👍

@ben-roling
Copy link
Contributor Author

ben-roling commented Nov 3, 2022

Added the "NotOnClasspath" tests in a place where it looked like they belonged. Need to see where any other tests would best fit.

@jack-berg
Copy link
Member

I've added a commit that declares logging-otlp as experimental in the docs and adds missing test coverage.

@jack-berg
Copy link
Member

@jkwatson any remaining concerns about merging this?

@jack-berg jack-berg merged commit 7dd5c1c into open-telemetry:main Nov 28, 2022
dmarkwat pushed a commit to dmarkwat/opentelemetry-java that referenced this pull request Dec 30, 2022
* Add autoconfigure support for logging-otlp

* Add NotOnClasspath tests

* Fix formatting

* Declare logging-otlp as experimental, add unit test

* Spotless

Co-authored-by: Jack Berg <[email protected]>
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.

5 participants