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

[instrumentation] Uprade to the latest OTEL #28811

Merged
merged 9 commits into from
Mar 11, 2024

Conversation

mpodwysocki
Copy link
Member

Packages impacted by this PR

  • @azure/opentelemetry-instrumentation-azure-sdk

Issues associated with this PR

Describe the problem that is addressed by this PR

Updates to latest OTEL

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 impacted package name to the issue description
  • Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
  • Added a changelog (if necessary)

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@@ -270,7 +270,7 @@ describe("OpenTelemetryInstrumenter", () => {

// Function syntax
instrumenter.withContext(context.active(), function (this: any) {
assert.notExists(this);
assert.isDefined(this);
Copy link
Member

Choose a reason for hiding this comment

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

This seems weird to me, but I don't want to block the PR. Feel free to pend the test and assign an issue to me to review it more closely. It's been like 2 years 😄

Copy link
Member

Choose a reason for hiding this comment

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

yeah feels like it should be undefined as it's not being explicitly bound to anything?

Copy link
Member

@hectorhdzg hectorhdzg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for taking care of this

@@ -270,7 +270,7 @@ describe("OpenTelemetryInstrumenter", () => {

// Function syntax
instrumenter.withContext(context.active(), function (this: any) {
assert.notExists(this);
assert.isDefined(this);
Copy link
Member

Choose a reason for hiding this comment

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

yeah feels like it should be undefined as it's not being explicitly bound to anything?

@xirzec
Copy link
Member

xirzec commented Mar 11, 2024

/check-enforcer reset

@xirzec
Copy link
Member

xirzec commented Mar 11, 2024

/check-enforcer evaluate

@xirzec
Copy link
Member

xirzec commented Mar 11, 2024

@microsoft-github-policy-service rerun

@xirzec
Copy link
Member

xirzec commented Mar 11, 2024

/check-enforcer reset

@xirzec
Copy link
Member

xirzec commented Mar 11, 2024

/check-enforcer evaluate

@mpodwysocki mpodwysocki merged commit 15f13e9 into main Mar 11, 2024
34 checks passed
@mpodwysocki mpodwysocki deleted the feat/otel-monitor-instrument branch March 11, 2024 18:16
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.

6 participants