Include schema_url in CachedInstrumentation by sudol · Pull Request #259 · open-telemetry/opentelemetry-php-contrib

sudol · 2024-04-26T16:38:55Z

This change is in reference to open-telemetry/opentelemetry-php#1290 and is in line with other Auto Instrumentation implementations.

opentelemetry-php-contrib/src/Instrumentation/Guzzle/src/GuzzleInstrumentation.php

Line 31 in 491827d

    
           $instrumentation = new CachedInstrumentation('io.opentelemetry.contrib.php.guzzle', schemaUrl: TraceAttributes::SCHEMA_URL);

opentelemetry-php-contrib/src/Instrumentation/ExtAmqp/src/ExtAmqpInstrumentation.php

Lines 27 to 30 in 491827d

    
           $instrumentation = new CachedInstrumentation( 
        
               'io.opentelemetry.contrib.php.ext_amqp', 
        
               InstalledVersions::getVersion('open-telemetry/opentelemetry-auto-ext-amqp'), 
        
               TraceAttributes::SCHEMA_URL,

welcome · 2024-04-26T16:38:57Z

Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one.

codecov · 2024-04-27T04:19:46Z

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 83.50%. Comparing base (2a601c3) to head (968d995).
Report is 12 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #259      +/-   ##
============================================
- Coverage     84.38%   83.50%   -0.89%     
- Complexity      983     1057      +74     
============================================
  Files            89      104      +15     
  Lines          3946     4388     +442     
============================================
+ Hits           3330     3664     +334     
- Misses          616      724     +108

Flag	Coverage Δ
7.4	`?`
8.0	`?`
8.1	`?`
8.2	`?`
8.3	`?`
Aws:7.4	`86.02% <ø> (?)`
Aws:8.0	`85.75% <ø> (?)`
Aws:8.1	`85.75% <ø> (?)`
Aws:8.2	`85.75% <ø> (?)`
Aws:8.3	`85.75% <ø> (?)`
Context/Swoole:7.4	`0.00% <ø> (?)`
Context/Swoole:8.0	`0.00% <ø> (?)`
Context/Swoole:8.1	`0.00% <ø> (?)`
Context/Swoole:8.2	`0.00% <ø> (?)`
Context/Swoole:8.3	`0.00% <ø> (?)`
Instrumentation/CakePHP:8.0	`93.47% <ø> (?)`
Instrumentation/CakePHP:8.1	`93.47% <ø> (?)`
Instrumentation/CakePHP:8.2	`93.47% <ø> (?)`
Instrumentation/CakePHP:8.3	`93.47% <ø> (?)`
Instrumentation/CodeIgniter:8.0	`75.86% <ø> (?)`
Instrumentation/CodeIgniter:8.1	`75.86% <ø> (?)`
Instrumentation/CodeIgniter:8.2	`75.86% <ø> (?)`
Instrumentation/CodeIgniter:8.3	`75.86% <ø> (?)`
Instrumentation/ExtAmqp:8.2	`89.58% <ø> (?)`
Instrumentation/ExtAmqp:8.3	`89.58% <ø> (?)`
Instrumentation/Guzzle:8.0	`72.60% <ø> (?)`
Instrumentation/Guzzle:8.1	`72.60% <ø> (?)`
Instrumentation/Guzzle:8.2	`72.60% <ø> (?)`
Instrumentation/Guzzle:8.3	`72.60% <ø> (?)`
Instrumentation/HttpAsyncClient:8.0	`81.33% <ø> (?)`
Instrumentation/HttpAsyncClient:8.1	`81.33% <ø> (?)`
Instrumentation/HttpAsyncClient:8.2	`81.33% <ø> (?)`
Instrumentation/HttpAsyncClient:8.3	`81.33% <ø> (?)`
Instrumentation/IO:8.2	`75.00% <ø> (?)`
Instrumentation/IO:8.3	`75.00% <ø> (?)`
Instrumentation/Laravel:8.0	`65.20% <ø> (?)`
Instrumentation/Laravel:8.1	`65.20% <ø> (?)`
Instrumentation/Laravel:8.2	`65.20% <ø> (?)`
Instrumentation/Laravel:8.3	`65.20% <ø> (?)`
Instrumentation/MongoDB:7.4	`80.55% <ø> (?)`
Instrumentation/MongoDB:8.0	`80.55% <ø> (?)`
Instrumentation/MongoDB:8.1	`80.55% <ø> (?)`
Instrumentation/MongoDB:8.2	`80.55% <ø> (?)`
Instrumentation/MongoDB:8.3	`80.55% <ø> (?)`
Instrumentation/OpenAIPHP:8.1	`86.82% <ø> (?)`
Instrumentation/OpenAIPHP:8.2	`86.82% <ø> (?)`
Instrumentation/OpenAIPHP:8.3	`86.82% <ø> (?)`
Instrumentation/PDO:8.2	`97.46% <ø> (?)`
Instrumentation/PDO:8.3	`97.46% <ø> (?)`
Instrumentation/Psr14:8.0	`80.64% <ø> (?)`
Instrumentation/Psr14:8.1	`80.64% <ø> (?)`
Instrumentation/Psr14:8.2	`80.64% <ø> (?)`
Instrumentation/Psr14:8.3	`80.64% <ø> (?)`
Instrumentation/Psr15:8.0	`93.50% <ø> (?)`
Instrumentation/Psr15:8.1	`93.50% <ø> (?)`
Instrumentation/Psr15:8.2	`93.50% <ø> (?)`
Instrumentation/Psr15:8.3	`93.50% <ø> (?)`
Instrumentation/Psr16:8.0	`97.43% <ø> (?)`
Instrumentation/Psr16:8.1	`97.43% <ø> (?)`
Instrumentation/Psr16:8.2	`97.43% <ø> (?)`
Instrumentation/Psr16:8.3	`97.43% <ø> (?)`
Instrumentation/Psr18:8.0	`82.08% <ø> (?)`
Instrumentation/Psr18:8.1	`82.08% <ø> (?)`
Instrumentation/Psr18:8.2	`82.08% <ø> (?)`
Instrumentation/Psr18:8.3	`82.08% <ø> (?)`
Instrumentation/Psr3:8.0	`63.51% <ø> (?)`
Instrumentation/Psr3:8.1	`63.51% <ø> (?)`
Instrumentation/Psr3:8.2	`63.51% <ø> (?)`
Instrumentation/Psr3:8.3	`63.51% <ø> (?)`
Instrumentation/Psr6:8.0	`97.61% <ø> (?)`
Instrumentation/Psr6:8.1	`97.61% <ø> (?)`
Instrumentation/Psr6:8.2	`97.61% <ø> (?)`
Instrumentation/Psr6:8.3	`97.61% <ø> (?)`
Instrumentation/Slim:8.0	`88.14% <0.00%> (?)`
Instrumentation/Slim:8.1	`88.88% <0.00%> (?)`
Instrumentation/Slim:8.2	`88.88% <0.00%> (?)`
Instrumentation/Slim:8.3	`88.88% <0.00%> (?)`
Instrumentation/Symfony:8.0	`93.35% <ø> (?)`
Instrumentation/Symfony:8.1	`93.35% <ø> (?)`
Instrumentation/Symfony:8.2	`93.35% <ø> (?)`
Instrumentation/Symfony:8.3	`93.35% <ø> (?)`
Instrumentation/Yii:8.0	`79.82% <ø> (?)`
Instrumentation/Yii:8.1	`79.82% <ø> (?)`
Instrumentation/Yii:8.2	`79.82% <ø> (?)`
Instrumentation/Yii:8.3	`79.82% <ø> (?)`
Logs/Monolog:7.4	`100.00% <ø> (?)`
Logs/Monolog:8.0	`100.00% <ø> (?)`
Logs/Monolog:8.1	`100.00% <ø> (?)`
Logs/Monolog:8.2	`100.00% <ø> (?)`
Logs/Monolog:8.3	`100.00% <ø> (?)`
Propagation/ServerTiming:8.0	`100.00% <ø> (?)`
Propagation/ServerTiming:8.1	`100.00% <ø> (?)`
Propagation/ServerTiming:8.2	`100.00% <ø> (?)`
Propagation/ServerTiming:8.3	`100.00% <ø> (?)`
Propagation/TraceResponse:7.4	`100.00% <ø> (?)`
Propagation/TraceResponse:8.0	`100.00% <ø> (?)`
Propagation/TraceResponse:8.1	`100.00% <ø> (?)`
Propagation/TraceResponse:8.2	`100.00% <ø> (?)`
Propagation/TraceResponse:8.3	`100.00% <ø> (?)`
ResourceDetectors/Azure:7.4	`91.66% <ø> (?)`
ResourceDetectors/Azure:8.0	`91.66% <ø> (?)`
ResourceDetectors/Azure:8.1	`91.66% <ø> (?)`
ResourceDetectors/Azure:8.2	`91.66% <ø> (?)`
ResourceDetectors/Azure:8.3	`91.66% <ø> (?)`
ResourceDetectors/Container:8.0	`93.02% <ø> (?)`
ResourceDetectors/Container:8.1	`93.02% <ø> (?)`
ResourceDetectors/Container:8.2	`93.02% <ø> (?)`
ResourceDetectors/Container:8.3	`93.02% <ø> (?)`
Shims/OpenTracing:7.4	`92.99% <ø> (?)`
Shims/OpenTracing:8.0	`92.99% <ø> (?)`
Shims/OpenTracing:8.1	`92.99% <ø> (?)`
Shims/OpenTracing:8.2	`92.99% <ø> (?)`
Shims/OpenTracing:8.3	`92.99% <ø> (?)`
Symfony:7.4	`88.43% <ø> (?)`
Symfony:8.0	`88.20% <ø> (?)`
Symfony:8.1	`88.20% <ø> (?)`
Symfony:8.2	`88.20% <ø> (?)`
Symfony:8.3	`88.20% <ø> (?)`

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

Files	Coverage Δ
...c/Instrumentation/Slim/src/SlimInstrumentation.php	`89.69% <0.00%> (+6.18%)`	⬆️

... and 8 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

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

ChrisLightfootWild · 2024-04-29T08:49:09Z

    public static function register(): void
    {
-        $instrumentation = new CachedInstrumentation('io.opentelemetry.contrib.php.slim');
+        $instrumentation = new CachedInstrumentation('io.opentelemetry.contrib.php.slim', schemaUrl: TraceAttributes::SCHEMA_URL);


Should this be explicitly set as the schema that it was created against?

It may potentially be invalid against future versions of the spec 🤔

You're right, I hadn't considered this. Since this package (opentelemetry-auto-slim), pulls in open-telemetry/sem-conv, which is the source of TraceAttributes, is it fair to assume that this package is compatible with whatever version of the schema is defined in open-telemetry/sem-conv?

Apologies for my delayed response to this @sudol

I have had a look through the documentation, and I think this may be the relevant part that was somewhere in the back of my mind: Schema-File Driven Telemetry Producers

I think that we would be at risk of changing the schema version URL in a potentially unsafe way if TraceAttributes::SCHEMA_URL is used as is.

@brettmc did mention in the SIG this week that maybe we could provide a series of constants for these versions instead (tagged for his input/thoughts on this!)

how does this look? open-telemetry/opentelemetry-php#1301

Nice @brettmc! I added a comment on there for discussion

My interpretation of the spec re: schema_url is that we should explicitly set it to the latest schema version (1.25.0), confirm that we're setting the values correctly per that version, then leave it alone.

@sudol - are you planning on making this update?

@sudol - use https://opentelemetry.io/schemas/1.25.0, and the version should not change again after that.

Ah yes. Sorry, I misunderstood. Yes I will get this updated tomorrow.

brettmc · 2024-05-02T09:04:39Z

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/telemetry-stability.md#schema-file-driven-telemetry-producers was just pointed out in the spec channel... specifically:

Stable instrumentations that include the Schema URL in the produced telemetry are called Schema-File Driven Telemetry Producers.
Such instrumentations are prohibited from changing the produced telemetry until the moratorium on relying on schema transformations for telemetry stability is lifted

The restrictions aren't any better for instrumentations that don't include a schema URL, though...

ChrisLightfootWild · 2024-05-02T16:38:29Z

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/telemetry-stability.md#schema-file-driven-telemetry-producers was just pointed out in the spec channel... specifically:

Stable instrumentations that include the Schema URL in the produced telemetry are called Schema-File Driven Telemetry Producers.
Such instrumentations are prohibited from changing the produced telemetry until the moratorium on relying on schema transformations for telemetry stability is lifted

The restrictions aren't any better for instrumentations that don't include a schema URL, though...

Happy to go with your judgement either way!

Update SlimInstrumentation.php

f47a064

sudol requested a review from a team April 26, 2024 16:38

brettmc approved these changes Apr 27, 2024

View reviewed changes

ChrisLightfootWild reviewed Apr 29, 2024

View reviewed changes

Hardcoded the Schema version to 1.25.0

968d995

sudol requested a review from brettmc May 16, 2024 04:01

brettmc approved these changes May 16, 2024

View reviewed changes

brettmc merged commit 900a435 into open-telemetry:main May 16, 2024

Nevay mentioned this pull request May 17, 2024

[opentelemetry-php-instrumentation] schema_url with fixed value open-telemetry/opentelemetry-php#1312

Closed

weslenteche mentioned this pull request Jun 20, 2024

Added schema URL for all packages lower than 1.0.0 #273

Merged

	$instrumentation = new CachedInstrumentation(
	'io.opentelemetry.contrib.php.ext_amqp',
	InstalledVersions::getVersion('open-telemetry/opentelemetry-auto-ext-amqp'),
	TraceAttributes::SCHEMA_URL,

Conversation

sudol commented Apr 26, 2024

Uh oh!

welcome Bot commented Apr 26, 2024

Uh oh!

codecov Bot commented Apr 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettmc commented May 2, 2024

Uh oh!

ChrisLightfootWild commented May 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Apr 27, 2024 •

edited

Loading