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

[tracing] Add support for addEvent #31162

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

maorleger
Copy link
Member

@maorleger maorleger commented Sep 19, 2024

Packages impacted by this PR

@azure/core-tracing
@azure/opentelemetry-instrumentation-azure-sdk
@typespec/ts-http-runtime

Issues associated with this PR

Resolves #31158

Describe the problem that is addressed by this PR

OpenTelemetry supports adding events to a span, and part of the gen-ai
specification expects events to be added on a span.

This PR adds support for the addEvent API in our core-tracing library as well
as our otel-sdk.

Adding a required field is a breaking change, so this is added as optional. The
implementation will need to use the safe-navigation operator syntax or check for
the function's existence before calling it.

In passing, I am updating some of our internal transformations to use OTel's APIs instead.

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Are there test cases added in this PR? (If not, why?)

Provide a list of related PRs (if any)

Command used to generate this PR: (Applicable only to SDK release request PRs)

Checklists

  • Added a changelog (if necessary).
  • Unbranded core

@azure-sdk
Copy link
Collaborator

azure-sdk commented Sep 19, 2024

API change check

APIView has identified API level changes in this PR and created following API reviews.

@azure/core-tracing
@typespec/ts-http-runtime

@maorleger maorleger marked this pull request as ready for review September 19, 2024 14:47
@mikeharder mikeharder added triage and removed triage labels Sep 19, 2024
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Minor nit

sdk/core/core-tracing/test/instrumenter.spec.ts Outdated Show resolved Hide resolved
sdk/core/ts-http-runtime/test/tracing/instrumenter.spec.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@jeremymeng jeremymeng left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[tracing] Add support for addEvent
5 participants