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

Logging sink api #1421

Closed
wants to merge 2 commits into from
Closed

Conversation

zenmoto
Copy link
Contributor

@zenmoto zenmoto commented Jul 15, 2020

This is an attempt at an initial log delivery API. It's not targeted as a replacement for a user-level logging API, instead it's focused on taking log entries and passing them on to an SDK for processing and delivery. The model is based off of open-telemetry/opentelemetry-proto#151.

@codecov
Copy link

codecov bot commented Jul 15, 2020

Codecov Report

Merging #1421 into master will decrease coverage by 0.13%.
The diff coverage is 87.21%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1421      +/-   ##
============================================
- Coverage     92.11%   91.97%   -0.14%     
- Complexity      904      930      +26     
============================================
  Files           115      120       +5     
  Lines          3258     3391     +133     
  Branches        265      266       +1     
============================================
+ Hits           3001     3119     +118     
- Misses          173      187      +14     
- Partials         84       85       +1     
Impacted Files Coverage Δ Complexity Δ
.../src/main/java/io/opentelemetry/OpenTelemetry.java 95.23% <71.42%> (-2.98%) 24.00 <2.00> (+2.00) ⬇️
...rc/main/java/io/opentelemetry/common/AnyValue.java 72.72% <72.72%> (ø) 7.00 <7.00> (?)
.../io/opentelemetry/logs/DefaultLogSinkProvider.java 77.77% <77.77%> (ø) 5.00 <5.00> (?)
...src/main/java/io/opentelemetry/logs/LogRecord.java 96.66% <96.66%> (ø) 0.00 <0.00> (?)
...n/java/io/opentelemetry/logs/DefaultLogRecord.java 100.00% <100.00%> (ø) 10.00 <10.00> (?)
...c/main/java/io/opentelemetry/logs/NoOpLogSink.java 100.00% <100.00%> (ø) 2.00 <2.00> (?)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb04118...0939fc6. Read the comment docs.

* @since 0.7.0
*/
@Immutable
public abstract class AnyValue {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have a proto version of this class already. is there a reason we can't use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We definitely have it in opentelemetry-proto, but I believe that api does not have a dependency on proto. I could be wrong- I'm trying to mirror how we are doing it with AttributeValue.

@jkwatson
Copy link
Contributor

If this isn't meant to be a user-level logging API, why have it accessible as a user-level instrumentation API? Users will definitely be confused if it's there, but they aren't supposed to use it.

@zenmoto
Copy link
Contributor Author

zenmoto commented Jul 15, 2020

It's not that it can't be used as a logging API, but it's not going to be ergonomic. This is to enable adapters for existing logging libraries to output through OpenTelemetry. It would also be the foundation for an ergonomic logging library, but I think that's going to be easier to work through when we have functional plumbing.

@jkwatson
Copy link
Contributor

I don't think we should add a logging API until it has made it into the official specifications. Especially before GA. I believe the agreement is that we will not GA with a logging API. Please point me somewhere else if I am wrong about that.

@bogdandrutu
Copy link
Member

@zenmoto to make progress, would strongly suggest to start in one of the extensions directories.

@zenmoto
Copy link
Contributor Author

zenmoto commented Jul 22, 2020

Thank you @bogdandrutu - I'll close this and take that approach instead.

@zenmoto zenmoto closed this Jul 22, 2020
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.

3 participants