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 KeyValue support for Observation.Event #5238

Open
lenin-jaganathan opened this issue Jul 1, 2024 · 5 comments
Open

Add KeyValue support for Observation.Event #5238

lenin-jaganathan opened this issue Jul 1, 2024 · 5 comments
Labels
enhancement A general enhancement help wanted An issue that a contributor can help us with module: micrometer-observation
Milestone

Comments

@lenin-jaganathan
Copy link
Contributor

Please describe the feature request.
Currently, Observation.Event supports only providing a name (and contextual name). The enhancement request is to allow the addition of low cardinality and high cardinality tags (aka attributes) to events.

Rationale
This would allow the Observation handlers to use them appropriately for use cases like tagging the counters created from the events with more informative tags, key value-based spans/logging systems can use this to enhance search capabilities and enrich events with additional contextual information, etc.
Adding support for tags in events would also enable places where certain tags are associated with an event to be flushed immediately and remove references. Today, such information can only be added to Observation which might be long-lived. Also, we cannot add too many low cardinal tags to the observation as that would make it emit timers with too many attributes. Doing so in events would ease these off from timers and add to counters which are relatively cheaper compared to Timers.

@jonatan-ivanov
Copy link
Member

jonatan-ivanov commented Jul 1, 2024

Thanks for the issue!
Could you please provide us a concrete real-world example where you would use this?

I think providing a simple implementation for this wouldn't be too hard, I'm a bit concerned though about two things:

  • Performance: in order to have KeyValues on events, we need to maintain two containers on each of them, I'm not sure how bad is it though, hopefully it's not too bad.
  • Breaking implementations of the Event interface: if we add new (non-default) methods to it (getter/setter), existing implementations can be broken, for example if somebody did something like this: GrpcServerEvents

/cc @ThomasVitale @jzheaux

@lenin-jaganathan lenin-jaganathan changed the title Add attribute support for Observation.Event Add KeyValue support for Observation.Event Jul 2, 2024
@lenin-jaganathan
Copy link
Contributor Author

lenin-jaganathan commented Jul 2, 2024

Sure. There are multiple places where we can use the KeyValue associated with an event. I am sharing some of the use-cases with each Observability signals that can be derived from them.

  1. Metrics: Currently, a counter is created for each event. This counter includes all the low cardinality tags in the context which seem to add way too many tags to the counter created. For instance, a counter for HTTP "receive" might not need all the information that was added when sending the request. Also, both send and receive might not need all the information associated with HTTP observation. Also, to get additional tags in Counter we need to add the tags in observation which then gets added to all the other events and more importantly to the timer as well, which makes the timer costlier and heavier.
  2. Logs: This is where we see more scope for events. We have a transactional logging system where events are tied to transactions. The above case with metrics apply here as well. Especially with high cardinality tags and 10's of events it make sense to add them to events instead of the entire observation itself. Today, adding them to observation makes it very very heavy
    and it also gets attached to other events if we try to use context to get data. Instead of that, if events support KeyValue we can keep them away from context and make context very lean. Only context gets propagated across thread boundaries and events can be flushed out as when they are created and the data can stay with them instead of being in the entire context. If we try to avoid getting KeyValue from context, events really make little sense with just the name part of them.

@shakuzen shakuzen added enhancement A general enhancement help wanted An issue that a contributor can help us with module: micrometer-observation and removed waiting-for-triage labels Jul 18, 2024
@shakuzen shakuzen added this to the 1.x milestone Jul 18, 2024
@shakuzen
Copy link
Member

I've gone ahead and marked this as help wanted. I think there may be some tricky things to work out in the details of implementing this, but if someone is interested in working on this, feel free to discuss here or send a pull request.

@ThomasVitale
Copy link

Thank you, @shakuzen! I want to study a bit deeper the Micrometer implementation for Spans first, and then I'll see if I can volunteer for helping with this feature.

My main use case right now for this feature would be to instrument LLM-powered applications and optionally include the prompt and completion content in Span events. Span attributes would not be the best choice for such data because they could be too long, whereas Span attributes are better suitable for longer content (such as exception stacktrace). That's also a requirement to adhere to the OpenTelemetry Semantic Conventions for Generative AI (see: https://opentelemetry.io/docs/specs/semconv/gen-ai/gen-ai-spans/#events).

I'm currently working on instrumenting and configuring observability for Spring AI (first part already delivered in spring-projects/spring-ai#954). Eventually, this feature would be used there.

@ThomasVitale
Copy link

ThomasVitale commented Aug 18, 2024

@shakuzen I've started doing some analysis to figure out the size of the task. One question I have so far is how you would like to support the different data types for event attributes in a span. OpenTelemetry relies on an Attributes class, which allows to use several data types, including an "array version" for each of them. The current Micrometer Span interface explicitly defines methods for each data type (but no support for arrays). And I can see the Micrometer KeyValue abstraction is not used in the Tracing project (only in Core). Would it be ok to use it for passing event attributes?

https://github.com/micrometer-metrics/tracing/blob/main/micrometer-tracing/src/main/java/io/micrometer/tracing/Span.java#L152

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement help wanted An issue that a contributor can help us with module: micrometer-observation
Projects
None yet
Development

No branches or pull requests

4 participants