Prototype: User facing logs API (including emit event)

[trask]

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

[jack-berg]

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

[codecov]

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]

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.

[breedx-splk]

I like it, no real surprises here.

[breedx-splk]

I think we normalized around "body" didn't we?

Suggested change

	/** Put the given {@code key} and {@code value} in the payload. */
	/** Put the given {@code key} and {@code value} in the event body. */

[breedx-splk]

nit: I wish this was encapsulated into DefaultLogger.eventBuilder(eventName)...

[trask]

sorry, I didn't follow what you mean here?

[breedx-splk]

oh, just literally that I wish there was a static method on DefaultLogger called eventBuilder(name) that would prevent us from having to go thru the instance here.

[lmolkova]

It's a feedback to the existing interface - it seems impossible to add a value without key (e.g. a plain string as a body or an array<Value>) and inconvenient to add a value that's created somewhere else. But it should be easy to address in the future.

Assuming I have some existing POJO for event payload, it'd prefer to do something like

logger.builder("test-event")
      .put(event.toValue())
      .emit();

and now it's not super-convenient:

logger.builder("test-event")
      .put("id", Value.of(event.getId()))
      .put("finishReasons", Value.of(Arrays.stream(event.getFinishReasons()).map(Value::of).toArray(Value[]::new)))
      .put("message", event.getMessage().toValue())
      .emit();

Full example: https://gist.github.com/lmolkova/1e7aae49483e411a5c7e633490202ee9

[jack-berg]

Do we know of instances where the event payload isn't a map? We can support this, but it seems good to put everything in a map. That way, every piece of data minimally has a key which helps interpret the contents.

[lmolkova]

We might introduce some GenAI events with array (of maps) body. Another example is someone recording an arbitrary HTTP request/response body as a string or byte array.

[trask]

@jack-berg I was worried that the put() methods were confusing whether they applied to attributes or body fields.

what do you think of removing them and just having setBody()?

I think the body is a "unit" which you will always want to set in a single place (as opposed to setting fields into it over time), so I'm not sure there is much downside

and I think at some point we want to be able to generate objects from semconv to encapsulate the bodies, which will still fit nicely with a single call to (some kind of) setBody() method

[jack-berg]

Initially there was only setBody(Value). I received feedback that the ergonomics of this really suck for users. The put(String, T) overloads represented syntactic sugar with the goal of making the user ergonomics reasonably good.

I still think put(String, T) overloads do meaningfully improve the experience vs. put(Value.of(Map.of("key", Value.of("value"))), and I still think that bodies which are maps are the common case.

What about having both put(String, T) overloads, and a setBody(Value) escape hatch? We can document that calling setBody(Value) erases any previous calls to put(String, T), and calling put(String, T) erases any previous calls to setBody(Value), and log warnings if either happens. Essentially, last win semantics with logging if anything gets overridden because its usually an error.

[trask]

I received feedback that the ergonomics of this really suck for users.

Could the ergonomics be improved by something like this?

setBody(ValueBuilder.put(...)
          .put(...)
          .put(...)
          .build()

The main reason I don't love the .put(...) methods on the EventBuilder is because it feels non-obvious whether those are putting body fields or attributes.

[breedx-splk]

Do we know of instances where the event payload isn't a map? We can support this, but it seems good to put everything in a map. That way, every piece of data minimally has a key which helps interpret the contents.

I've been biting my tongue on this for a long time now, because I didn't want to get in the way of progress...but I definitely think it's over-simplifying to force the body to a map. It was very obvious back when it was spelled AnyValue because it literally read in English like "Any value....as long as it's a map". 😉

There are definitely cases where forcing the body content into a fielded map/dictionary causes redundancy. We obviously don't have many events yet in semconv due to the chaos, but one specific example is the session.end event (link). The semantics could have been that the body contains the string representation of the session id, and that's it. But because of the forced keys/map nature of the body, we've had to cram the session id value into a body field called session.id.

The name of the event should be enough to signify what the content of the body contains, without additional hints or unnecessary bulky/redundant fielding. It would then be up to the event designer to decide how simple or complicated it needs to be.

As another contrived example, let's say a mobile team has built an app, and they want to send an event to indicate a new high score. One semantic approach could be to just define an event that contains the high score value as a long:

{
  event.name: mygame.highscore,
  body: 1250000 
}

and maybe that's sufficient for their uses....while another game might have more detailed requirements:

{
  event.name: game2.highscore,
  body: {
    score: 1999999,
    level: 17,
    initials: jjp
  }
}

I think both are valid uses cases for events. I think that if you always force a map, you'll see things like this:

{
  event.name: game.highscore,
  body: { 
    value: 123123999
  }
}

which IMO has a pretty bad smell. I think we should leave the power/flexibility there and allow the event designers to use the model that suits them best.

[jack-berg]

I'm happy as long as the ergonomics remain good. Adding a ValueBuilder seems like a good approach. Although I think we only need builders for maps and arrays.

[jkwatson]

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]

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())

[jkwatson]

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]

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]

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.