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.

Sign up for GitHub

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

Jump to bottom

Add Event Emitter with Data #122

Closed
wants to merge 6 commits into from
Closed

Add Event Emitter with Data #122

wants to merge 6 commits into from

Conversation

tonzhan2
Copy link

@tonzhan2 tonzhan2 commented Oct 19, 2023 •
edited
Loading

Clean copy of this PR #100 with updated changes from master

@tonzhan2 tonzhan2 marked this pull request as ready for review October 21, 2023 20:18
@tonzhan2 tonzhan2 requested a review from a team October 21, 2023 20:18
@breedx-splk breedx-splk mentioned this pull request Oct 24, 2023
Merged
@breedx-splk
Copy link
Contributor

Hi @tonzhan2 . Just wanted to touch base and let you know we haven't dropped this -- it's very much just in flux while the special events working group figures out some additional basics that might directly impact this PR. I appreciate your patience around this effort.

If you want to keep track of the work over there, here is the public tracking notes doc: https://docs.google.com/document/d/1BKjQWP32FXL9g1cGbyj7DMXV1Uq_RL8_78rWaMBhN0A/edit#heading=h.1nz72gv5evo2

Thanks!

marandaneto
marandaneto reviewed Feb 14, 2024
View reviewed changes
instrumentation/src/main/java/io/opentelemetry/android/emitter/EventEmitterWithData.java
} else if (val instanceof Boolean) {
builder.put(AttributeKey.booleanKey(key), (Boolean) val);
} else {
builder.put(AttributeKey.stringKey(key), val.toString());
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to consider the list types? https://github.com/open-telemetry/opentelemetry-java/blob/48d41d3a5aa27ff8e5e4033ef0c8c3ba0e53f683/api/all/src/main/java/io/opentelemetry/api/common/AttributeKey.java#L49-L67

@breedx-splk
Copy link
Contributor

With all respect... Given that:

  • this PR is stale and the repo has been reorganized from underneath it
  • core #6318 is likely to merge soon
  • ...and we want consistency/reuse of the upstream/standard otel APIs

I'm going to close this PR with the expectation that we will soon be able to leverage the new "data" (complex types) API from core.

Feel free to reopen if this seems off-base. Thanks again for the submission and for blazing the trail. 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marandaneto marandaneto marandaneto left review comments

@surbhiia surbhiia surbhiia left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tonzhan2 @breedx-splk @marandaneto @surbhiia