Skip to content

[instrumentation] - Add integration tests#20462

Merged
maorleger merged 9 commits intoAzure:mainfrom
maorleger:integration-test-opentelemetry
Feb 28, 2022
Merged

[instrumentation] - Add integration tests#20462
maorleger merged 9 commits intoAzure:mainfrom
maorleger:integration-test-opentelemetry

Conversation

@maorleger
Copy link
Copy Markdown
Member

Packages impacted by this PR

@azure/opentelemetry-instrumentation-azure-sdk

Issues associated with this PR

Resolves #18738

Describe the problem that is addressed by this PR

This PR solves the following:

  1. I would like to have an end-to-end test that uses our pipeline and ensures
    tracing works as expected. The idea here is to ensure we do not regress in the
    future when making core changes. I added a node-only integration test that uses
    core-rest-pipeline and a fake client.
  2. We currently maintain a TestTracer, TestTracerProvider, and TestSpan
    independently in multiple places. OpenTelemetry already provides testing utils
    in their base package. I replaced our Test* objects with an InMemorySpanExporter
    which allows us to grab all exported spans and inspect them.
  3. In-passing, I realized that for a bit there could be a drift between client
    packages that use the new naming conventions and core packages that use the old
    naming conventions to pull az.namespace into a span. While it doesn't feel
    appropriate to add compatibility code in our core packages, this package will be
    in beta for quite some time so I think it's fine to add it here temporarily.

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

Lots and lots

Checklists

  • Added impacted package name to the issue description.
  • Does this PR need 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).

Copy link
Copy Markdown
Member

@joheredi joheredi left a comment

Choose a reason for hiding this comment

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

Looks good, I like the new test and how you are verifying that the call chain is correctly traced. Just left a small comment about the outer function name that threw me off initially when reading the tests.

Copy link
Copy Markdown
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.

A few minor remarks, but I love that we're able to use more publicly provided goodies from OTel here.

Comment thread sdk/instrumentation/opentelemetry-instrumentation-azure-sdk/src/instrumenter.ts Outdated
@maorleger maorleger force-pushed the integration-test-opentelemetry branch from adc293d to 599cbcd Compare February 28, 2022 17:03
@maorleger maorleger merged commit fa4e3d1 into Azure:main Feb 28, 2022
@maorleger maorleger deleted the integration-test-opentelemetry branch February 28, 2022 19:19
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.

Integration tests for instrumentation-opentelemetry

3 participants