Skip to content

KeyValues on Observation.Event #6003

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shakuzen
Copy link
Member

@shakuzen shakuzen commented Mar 7, 2025

Opening for discussion related to implementing #5238. It's been implemented in a way that nothing should break.

I intentionally did not separate out high- vs low-cardinality keyvalues. I'd be interested to hear feedback on if it's needed and why.

I think most of the real impact is in how this will be handled: by our DefaultMeterObservationHandler, our default tracing handler, and other handlers.

@shakuzen shakuzen marked this pull request as draft March 7, 2025 09:46
@shakuzen
Copy link
Member Author

shakuzen commented Mar 7, 2025

Pinging @lenin-jaganathan and @ThomasVitale who were asking for this. I'm not sure whether this API serves the purpose you had in mind, but I figured it may be easier to start with something concrete to discuss.

* @return event
* @since 1.15.0
*/
static Event of(String name, String contextualName, long wallTime, Iterable<KeyValue> keyValues) {
Copy link
Member

Choose a reason for hiding this comment

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

I think in the handlers event keyvalues can be used:

  1. Low cardinality keyvalues can be attached to metrics: right now we create a counter, we can merge the low cardinality keyvalues of the observation with the the low cardinality keyvalues of the event.
  2. All keyvalues (low+high) can be attached to the annotation of the span (in case of zipkin we can encode it to k1=v1;k2=v2 format I think).

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.

2 participants