allow setting service name from auto instrumentation context #881

pdelewski · 2022-12-03T22:06:46Z

Currently there is no way to set service name from auto instrumentation context for zipkin and newrelic exporters.
We should read that from OTEL_SERVICE_NAME or any other configuration settings IMO.

codecov · 2022-12-03T22:15:38Z

Codecov Report

Merging #881 (c3b81ec) into main (02d9421) will increase coverage by 0.01%.
The diff coverage is 87.50%.

❗ Current head c3b81ec differs from pull request most recent head 31d358a. Consider uploading reports for the commit 31d358a to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #881      +/-   ##
============================================
+ Coverage     81.58%   81.59%   +0.01%     
  Complexity     2010     2010              
============================================
  Files           264      264              
  Lines          5223     5226       +3     
============================================
+ Hits           4261     4264       +3     
  Misses          962      962

Flag	Coverage Δ
7.4	`81.07% <86.95%> (+<0.01%)`	⬆️
8.0	`81.57% <87.50%> (+0.01%)`	⬆️
8.1	`81.71% <87.50%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/Contrib/Jaeger/HttpCollectorExporter.php	`100.00% <ø> (ø)`
src/Contrib/Newrelic/SpanExporterFactory.php	`0.00% <0.00%> (ø)`
src/Contrib/Zipkin/SpanExporterFactory.php	`0.00% <0.00%> (ø)`
src/Contrib/ZipkinToNewrelic/Exporter.php	`85.00% <50.00%> (ø)`
src/Contrib/Jaeger/AgentExporter.php	`100.00% <100.00%> (ø)`
src/Contrib/Jaeger/HttpSender.php	`100.00% <100.00%> (ø)`
src/Contrib/Newrelic/Exporter.php	`100.00% <100.00%> (ø)`
src/Contrib/Newrelic/SpanConverter.php	`96.29% <100.00%> (+0.14%)`	⬆️
src/Contrib/Zipkin/Exporter.php	`100.00% <100.00%> (ø)`
src/Contrib/Zipkin/SpanConverter.php	`96.29% <100.00%> (+0.03%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02d9421...31d358a. Read the comment docs.

Nevay · 2022-12-03T23:46:57Z

IMO the service name should not be a constructor argument and should be read in ::export() from SpanDataInterface::getResource() instead.

pdelewski · 2022-12-03T23:51:50Z

IMO the service name should not be a constructor argument and should be read in ::export() from SpanDataInterface::getResource() instead.

Thx. That's the discussion I wanted to start opening this PR

brettmc · 2022-12-04T05:00:19Z

I think the spec agrees with Nevay here; the objective of providing the name in the exporter is eventually to pass it through as a service name, but that should indeed come from a resource attribute: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/semantic_conventions/README.md#service
Since not all exporters accept a name, it looks like we should remove the constructor argument totally (assuming we can instead get the service name out of resource attributes)

pdelewski · 2022-12-05T10:54:03Z

IMO the service name should not be a constructor argument and should be read in ::export() from SpanDataInterface::getResource() instead.

@Nevay I'm currently looking into this code and have some concerns. export takes whole batch

public function export(iterable $batch, ?CancellationInterface $cancellation = null): FutureInterface

its execution leads to execution of private function convertSpan(SpanDataInterface $span): array.

Should not we read and update service.name here?

Nevay · 2022-12-05T11:36:43Z

Yes,

opentelemetry-php/src/Contrib/Zipkin/SpanConverter.php

Line 85 in 587bc5a

'serviceName' => $this->serviceName,

should be changed to something like

'serviceName' => $span->getResource()->getAttributes()->get(ResourceAttributes::SERVICE_NAME),

pdelewski · 2022-12-05T11:42:44Z

Yes,

opentelemetry-php/src/Contrib/Zipkin/SpanConverter.php

Line 85 in 587bc5a

'serviceName' => $this->serviceName,

should be changed to something like
'serviceName' => $span->getResource()->getAttributes()->get(ResourceAttributes::SERVICE_NAME),

Thx. That's exactly what I was thinking about

Nevay · 2022-12-05T18:34:48Z

src/Contrib/Zipkin/SpanConverter.php

@@ -78,11 +72,12 @@ private function convertSpan(SpanDataInterface $span): array
        $startTimestamp = TimeUtil::nanosToMicros($span->getStartEpochNanos());
        $endTimestamp = TimeUtil::nanosToMicros($span->getEndEpochNanos());

+        $serviceName =  $span->getResource()->getAttributes()->get(ResourceAttributes::SERVICE_NAME) ?? 'zipkin';


Must use default resource as fallback, spec:

Zipkin service name MUST be set to the value of the resource attribute: service.name. If no service.name is contained in a Span's Resource, it MUST be populated from the default Resource.

Yes, that's right, it's still work in progress

@Nevay fixed fallback

Nevay · 2022-12-06T19:36:52Z

src/Contrib/Newrelic/Exporter.php

@@ -23,25 +23,23 @@
 */
 class Exporter implements SpanExporterInterface
 {
+    public $name;


Should be removed.

brettmc · 2022-12-06T23:06:05Z

src/Contrib/Newrelic/SpanConverter.php

@@ -40,12 +35,16 @@ private function convertSpan(SpanDataInterface $span): array
        $startTimestamp = TimeUtil::nanosToMillis($span->getStartEpochNanos());
        $endTimestamp = TimeUtil::nanosToMillis($span->getEndEpochNanos());

+        $serviceName =  $span->getResource()->getAttributes()->get(ResourceAttributes::SERVICE_NAME)
+                        ??
+                        ResourceInfoFactory::defaultResource()->getAttributes()->get(ResourceAttributes::SERVICE_NAME);


depending on how many detectors are enabled (by default, all of them are), this is a bit of overhead we could avoid? Perhaps setting a default/fallback service name in the constructor?
(also applies to a couple of other converters that also do this)

brettmc · 2022-12-07T21:28:14Z

src/Contrib/Newrelic/SpanConverter.php

@@ -18,9 +20,9 @@ class SpanConverter implements SpanConverterInterface

    private string $serviceName;


a better name might be fallbackServiceName or defaultServiceName ?

brettmc · 2022-12-07T21:30:14Z

tests/Unit/Contrib/Zipkin/ZipkinSpanConverterTest.php

        $row = $converter->convert([$span])[0];

        $this->assertSame($span->getContext()->getSpanId(), $row['id']);
        $this->assertSame($span->getContext()->getTraceId(), $row['traceId']);
        $this->assertSame('1000000000000000', $row['parentId']);

-        $this->assertSame('test.name', $row['localEndpoint']['serviceName']);
+        $this->assertSame('unknown_service', $row['localEndpoint']['serviceName']);


you might need to merge in main branch, this is something I changed in a commit in the last day or so... unknown_service:php

yes, that's right.

pdelewski requested a review from a team December 3, 2022 22:06

pdelewski force-pushed the hardcoded-service-name branch from 66a0772 to 2a789e0 Compare December 3, 2022 22:13

pdelewski changed the title ~~allow setting service name from auto instrumentation context~~ WIP: allow setting service name from auto instrumentation context Dec 5, 2022

pdelewski changed the title ~~WIP: allow setting service name from auto instrumentation context~~ WIP allow setting service name from auto instrumentation context Dec 5, 2022

pdelewski force-pushed the hardcoded-service-name branch 3 times, most recently from 89bf331 to c81980d Compare December 5, 2022 17:27

Nevay reviewed Dec 5, 2022

View reviewed changes

pdelewski force-pushed the hardcoded-service-name branch 5 times, most recently from 1ce869f to 0b04c02 Compare December 6, 2022 11:50

pdelewski changed the title ~~WIP allow setting service name from auto instrumentation context~~ allow setting service name from auto instrumentation context Dec 6, 2022

Nevay reviewed Dec 6, 2022

View reviewed changes

pdelewski force-pushed the hardcoded-service-name branch 2 times, most recently from 7dec032 to 448ac3a Compare December 6, 2022 20:09

brettmc reviewed Dec 6, 2022

View reviewed changes

pdelewski force-pushed the hardcoded-service-name branch 2 times, most recently from 824c9eb to c3b81ec Compare December 7, 2022 12:00

brettmc reviewed Dec 7, 2022

View reviewed changes

pdelewski force-pushed the hardcoded-service-name branch from c3b81ec to 54845a0 Compare December 7, 2022 22:10

brettmc approved these changes Dec 7, 2022

View reviewed changes

allow setting service name from auto instrumentation context

31d358a

pdelewski force-pushed the hardcoded-service-name branch from 54845a0 to 31d358a Compare December 7, 2022 22:17

pdelewski merged commit 7bfc121 into open-telemetry:main Dec 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow setting service name from auto instrumentation context #881

allow setting service name from auto instrumentation context #881

pdelewski commented Dec 3, 2022

codecov bot commented Dec 3, 2022 •

edited

Loading

Nevay commented Dec 3, 2022

pdelewski commented Dec 3, 2022

brettmc commented Dec 4, 2022

pdelewski commented Dec 5, 2022 •

edited

Loading

Nevay commented Dec 5, 2022

pdelewski commented Dec 5, 2022

Nevay Dec 5, 2022

pdelewski Dec 5, 2022

pdelewski Dec 6, 2022

Nevay Dec 6, 2022

brettmc Dec 6, 2022 •

edited

Loading

pdelewski Dec 7, 2022

brettmc Dec 7, 2022

brettmc Dec 7, 2022

pdelewski Dec 7, 2022 •

edited

Loading

		@@ -18,9 +20,9 @@ class SpanConverter implements SpanConverterInterface

		private string $serviceName;

allow setting service name from auto instrumentation context #881

allow setting service name from auto instrumentation context #881

Conversation

pdelewski commented Dec 3, 2022

codecov bot commented Dec 3, 2022 • edited Loading

Codecov Report

Nevay commented Dec 3, 2022

pdelewski commented Dec 3, 2022

brettmc commented Dec 4, 2022

pdelewski commented Dec 5, 2022 • edited Loading

Nevay commented Dec 5, 2022

pdelewski commented Dec 5, 2022

Nevay Dec 5, 2022

Choose a reason for hiding this comment

pdelewski Dec 5, 2022

Choose a reason for hiding this comment

pdelewski Dec 6, 2022

Choose a reason for hiding this comment

Nevay Dec 6, 2022

Choose a reason for hiding this comment

brettmc Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

pdelewski Dec 7, 2022

Choose a reason for hiding this comment

brettmc Dec 7, 2022

Choose a reason for hiding this comment

brettmc Dec 7, 2022

Choose a reason for hiding this comment

pdelewski Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Dec 3, 2022 •

edited

Loading

pdelewski commented Dec 5, 2022 •

edited

Loading

brettmc Dec 6, 2022 •

edited

Loading

pdelewski Dec 7, 2022 •

edited

Loading