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

fix(exporter-jaeger): transform all links to jaeger reference #2731

Merged
merged 6 commits into from
Jan 31, 2022
Merged

fix(exporter-jaeger): transform all links to jaeger reference #2731

merged 6 commits into from
Jan 31, 2022

Conversation

blumamir
Copy link
Member

@blumamir blumamir commented Jan 24, 2022

Which problem is this PR solving?

Fixes #2718
Exporter jaeger is currently transforming and exporting only links that point to the parent span. This is in contrast to the spec:

OpenTelemetry Link(s) MUST be converted to SpanReference(s) in Jaeger, using FOLLOWS_FROM reference type

I don't know why it was added in the first place, but it looks like a bug.
References from other SDKs:
ruby - transform all links to jaeger references with no filter.
python - same, transform all links regardless of parent releationship
java - the same

Fixes # (issue)

  • In the jaeger exporter transform links, I removed theif that transform only links that point to span's parent. Now all links are transformed and exported to jaeger.

Short description of the changes

  • Modified the links transform
  • Updated the relevant test to verify the reference fields

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Update the unit test for transform
  • Created a demo app that export spans to Jaeger, and verify that before the change the link is missing in jaeger, and after the changed package is yarn linked, the span is shown in jaeger:
import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node';
import { SimpleSpanProcessor } from '@opentelemetry/sdk-trace-base';
import { JaegerExporter } from '@opentelemetry/exporter-jaeger';

const provider = new NodeTracerProvider();
provider.addSpanProcessor(new SimpleSpanProcessor(new JaegerExporter()));
provider.register();

const tracer = provider.getTracer('test');
const s1 = tracer.startSpan('span 1');
const s2 = tracer.startSpan('span 2', {
    links: [ {
        context: s1.spanContext()
    }]
});
s1.end();
s2.end();

provider.shutdown();

image

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated (no docs update needed)

@blumamir blumamir requested a review from a team January 24, 2022 09:30
@codecov
Copy link

codecov bot commented Jan 24, 2022

Codecov Report

Merging #2731 (5af18da) into main (3231cdc) will increase coverage by 1.26%.
The diff coverage is 100.00%.

❗ Current head 5af18da differs from pull request most recent head ff0efb8. Consider uploading reports for the commit ff0efb8 to get more accurate results

@@            Coverage Diff             @@
##             main    #2731      +/-   ##
==========================================
+ Coverage   92.01%   93.27%   +1.26%     
==========================================
  Files         147      159      +12     
  Lines        4706     5444     +738     
  Branches      990     1141     +151     
==========================================
+ Hits         4330     5078     +748     
+ Misses        376      366      -10     
Impacted Files Coverage Δ
...ges/opentelemetry-exporter-jaeger/src/transform.ts 100.00% <100.00%> (ø)
...s/exporter-trace-otlp-http/src/OTLPExporterBase.ts 100.00% <0.00%> (ø)
...exporter-metrics-otlp-http/src/transformMetrics.ts 95.65% <0.00%> (ø)
...es/opentelemetry-context-zone-peer-dep/src/util.ts 100.00% <0.00%> (ø)
...-instrumentation-fetch/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
packages/exporter-trace-otlp-http/src/transform.ts 88.69% <0.00%> (ø)
...mentation-xml-http-request/src/enums/EventNames.ts 100.00% <0.00%> (ø)
...ation-xml-http-request/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
...ry-context-zone-peer-dep/src/ZoneContextManager.ts 85.18% <0.00%> (ø)
... and 6 more

@vmarchaud vmarchaud added the bug Something isn't working label Jan 29, 2022
@vmarchaud vmarchaud merged commit a824578 into open-telemetry:main Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jaeger Exporter filters out links having a different link span id than the parent id
4 participants