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

Prototype: User facing logs API (including emit event) #6761

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

trask
Copy link
Member

@trask trask commented Oct 2, 2024

Just copy pasted over from the (to be previous) Event API as a first pass...

@trask trask force-pushed the user-facing-logs-api branch from be202ea to 26785ca Compare October 2, 2024 22:17
@jack-berg
Copy link
Member

Let me know when you want me to give this a proper review

@trask trask force-pushed the user-facing-logs-api branch from 60f5a2a to 51c8611 Compare October 2, 2024 22:28
Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 5.55556% with 51 lines in your changes missing coverage. Please review.

Project coverage is 90.26%. Comparing base (74579aa) to head (c11689e).

Files with missing lines Patch % Lines
...ava/io/opentelemetry/sdk/logs/SdkEventBuilder.java 0.00% 26 Missing ⚠️
.../java/io/opentelemetry/api/logs/DefaultLogger.java 11.11% 8 Missing ⚠️
...etry/api/incubator/logs/ExtendedDefaultLogger.java 11.11% 8 Missing ⚠️
...main/java/io/opentelemetry/sdk/logs/SdkLogger.java 16.66% 5 Missing ⚠️
.../main/java/io/opentelemetry/api/OpenTelemetry.java 0.00% 3 Missing ⚠️
...rc/main/java/io/opentelemetry/api/logs/Logger.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6761      +/-   ##
============================================
- Coverage     90.49%   90.26%   -0.23%     
  Complexity     6599     6599              
============================================
  Files           731      733       +2     
  Lines         19735    19789      +54     
  Branches       1935     1935              
============================================
+ Hits          17859    17863       +4     
- Misses         1285     1336      +51     
+ Partials        591      590       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@trask
Copy link
Member Author

trask commented Oct 3, 2024

Let me know when you want me to give this a proper review

I think it's ready for a basic review of the proposed API surface. I'll move it to the incubator module and add tests later on.

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

I like it, no real surprises here.

@jkwatson
Copy link
Contributor

I sure would like a convenience method that (I think) takes a name and some attributes, and skips the builder.
logger.emit("paymentReceived", Attributes.of("amount", 3.14, "currency", "Dollars"))

Or however we spell Attributes for the event API.

@trask
Copy link
Member Author

trask commented Oct 31, 2024

I sure would like a convenience method that (I think) takes a name and some attributes, and skips the builder. logger.emit("paymentReceived", Attributes.of("amount", 3.14, "currency", "Dollars"))

Or however we spell Attributes for the event API.

so like this?

logger.emit("paymentReceived", Value.builder().put("amount", 3.14).put("currency", "Dollars").build())

lmolkova pushed a commit to open-telemetry/opentelemetry-specification that referenced this pull request Nov 1, 2024
Continuing the work of implementing [OTEP
265](https://github.com/open-telemetry/oteps/blob/main/text/0265-event-vision.md),
this PR adds the EmitEvent method to the Log API.

Deprecating the experimental Event API and SDK can be handled in a
follow up PR.

Java prototype:
open-telemetry/opentelemetry-java#6761

---------

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Co-authored-by: Robert Pająk <pellared@hotmail.com>
@jkwatson
Copy link
Contributor

jkwatson commented Nov 2, 2024

I sure would like a convenience method that (I think) takes a name and some attributes, and skips the builder. logger.emit("paymentReceived", Attributes.of("amount", 3.14, "currency", "Dollars"))
Or however we spell Attributes for the event API.

so like this?

logger.emit("paymentReceived", Value.builder().put("amount", 3.14).put("currency", "Dollars").build())

We had an extensive discussion about this in the last Java SIG meeting, and it became very clear that it's not clear what should be attributes and what should be body, and how we even explain the difference to people, and how we should tell people to use them.

Whatever the results of that are, we should have an API that makes it really easy to do the recommended thing.

@lmolkova
Copy link
Contributor

lmolkova commented Nov 3, 2024

We had an extensive discussion about this in the last Java SIG meeting, and it became very clear that it's not clear what should be attributes and what should be body, and how we even explain the difference to people, and how we should tell people to use them.

Whatever the results of that are, we should have an API that makes it really easy to do the recommended thing.

It's probably more than API design problem - I don't think we have a clear guidance from semconv perspective on what should go to attributes vs body.

@jkwatson
Copy link
Contributor

jkwatson commented Nov 4, 2024

Whatever the results of that are, we should have an API that makes it really easy to do the recommended thing.

It's probably more than API design problem - I don't think we have a clear guidance from semconv perspective on what should go to attributes vs body.

I feel like we might be getting the cart ahead of the horse a bit, if we're not even sure (as a community) that we know how people are supposed to even be using the new APIs. For instance, if the body is the thing people should be thinking about, we can make that the front-and-center thing, but if it's attributes, then we should make that the easy thing. Until that's decided, I'm guessing we won't know how to build a good user-facing API.

@trask trask force-pushed the user-facing-logs-api branch from c11689e to 59862e4 Compare December 4, 2024 21:34
@trask
Copy link
Member Author

trask commented Dec 4, 2024

I feel like we might be getting the cart ahead of the horse a bit, if we're not even sure (as a community) that we know how people are supposed to even be using the new APIs.

I think it would be helpful at this point to move forward with the prototype / incubating API so people (@breedx-splk) can start using it as part of the effort to push the horse forward

@jack-berg
Copy link
Member

For instance, if the body is the thing people should be thinking about, we can make that the front-and-center thing, but if it's attributes, then we should make that the easy thing.

Body is the front and center thing. Evidence is that the semantic conventions are defining events in terms of fields of the body and not attributes. I think there's still outstanding question for end users producing custom events on when its appropriate to use attributes, but IMO its clear that body is priority.

@lmolkova
Copy link
Contributor

lmolkova commented Dec 4, 2024

Body is the front and center thing. Evidence is that the semantic conventions are defining events in terms of fields of the body and not attributes. I think there's still outstanding question for end users producing custom events on when its appropriate to use attributes, but IMO its clear that body is priority.

We have evidence in the both ways (feature-flags). The genai conventions you mentioned turned out problematic - some of the body fields are essential when querying things, but it's much harder to do using body fields. I'll propose to promote them to attributes. Plus there will be more indexable things that would go to attributes - see https://github.com/microsoft/opentelemetry-semantic-conventions/blob/e7276ff1bebb5914048d4843eeb78f20abe3de00/docs/gen-ai/gen-ai-events.md#event-gen_aisystemmessage as an example.

We don't really know how people would use events - we are still clarifying our own understanding of what goes where and why, but the answer seems that we need both body and attributes equally.

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