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

declarative config 0.3+ support #1452

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

brettmc
Copy link
Collaborator

@brettmc brettmc commented Dec 12, 2024

I was unable to implement 0.3.0 per-spec due to complexity in the metric reader section, which has since been fixed in open-telemetry/opentelemetry-configuration#148 so this updates to commit open-telemetry/opentelemetry-configuration@1645262 which is somewhere between 0.3 and 0.4

We can now parse the full kitchen-sink example (except for a warning about merging different schema versions).
Some dummy/placeholder component providers have been added to tests/Integration to cover components that are referenced in kitchen-sink.yaml but not implemented in our SDK.

since open-telemetry/opentelemetry-specification#4269 empty keys are allows, and NULL is no longer equivalent to unset
some of the components in the kitchen-sink config are not implemented, or are implemented in a different repo. for now, implement
no-op versions and log a warning if they're used.
as of this commit, we can fully parse the kitchen-sink.yaml config, which will become v0.4.0
Copy link

codecov bot commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 5.57491% with 271 lines in your changes missing coverage. Please review.

Project coverage is 72.47%. Comparing base (b515b6b) to head (953cb84).

Files with missing lines Patch % Lines
...nfiguration/Internal/ComponentProviderRegistry.php 0.00% 59 Missing ⚠️
.../Config/SDK/ComponentProvider/OpenTelemetrySdk.php 0.00% 42 Missing ⚠️
...nfig/SDK/Configuration/Internal/Node/NodeTrait.php 0.00% 24 Missing ⚠️
...on/Internal/NodeDefinition/ArrayNodeDefinition.php 0.00% 17 Missing ⚠️
...nfig/SDK/Configuration/Internal/Node/ArrayNode.php 0.00% 16 Missing ⚠️
...onfiguration/Internal/Node/PrototypedArrayNode.php 0.00% 16 Missing ⚠️
...on/Internal/NodeDefinition/NodeDefinitionTrait.php 0.00% 8 Missing ⚠️
src/Config/SDK/Configuration/Validation.php 0.00% 8 Missing ⚠️
...der/Instrumentation/General/HttpConfigProvider.php 0.00% 6 Missing ⚠️
...tProvider/InstrumentationConfigurationRegistry.php 0.00% 4 Missing ⚠️
... and 32 more
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1452      +/-   ##
============================================
- Coverage     73.27%   72.47%   -0.80%     
- Complexity     2683     2724      +41     
============================================
  Files           387      397      +10     
  Lines          8014     8128     +114     
============================================
+ Hits           5872     5891      +19     
- Misses         2142     2237      +95     
Flag Coverage Δ
8.1 ?
8.2 72.40% <5.57%> (-0.79%) ⬇️
8.3 72.39% <5.57%> (-0.79%) ⬇️
8.4 72.33% <5.57%> (-0.88%) ⬇️
8.5 72.40% <5.57%> (-0.81%) ⬇️

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

Files with missing lines Coverage Δ
.../Config/SDK/Configuration/ConfigurationFactory.php 100.00% <100.00%> (ø)
...guration/Internal/EnvSubstitutionNormalization.php 0.00% <ø> (ø)
src/SDK/Metrics/Stream/DeltaStorage.php 100.00% <ø> (ø)
...Provider/Propagator/TextMapPropagatorComposite.php 0.00% <0.00%> (ø)
...der/Instrumentation/General/PeerConfigProvider.php 0.00% <0.00%> (ø)
...omponentProvider/Logs/LogRecordExporterConsole.php 0.00% <0.00%> (ø)
...ComponentProvider/Logs/LogRecordProcessorBatch.php 0.00% <0.00%> (ø)
...omponentProvider/Logs/LogRecordProcessorSimple.php 0.00% <0.00%> (ø)
...entProvider/Metrics/AggregationResolverDefault.php 0.00% <0.00%> (ø)
...omponentProvider/Metrics/MetricExporterConsole.php 0.00% <0.00%> (ø)
... and 35 more

... and 5 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 b515b6b...953cb84. Read the comment docs.

@brettmc brettmc marked this pull request as ready for review December 12, 2024 05:26
@brettmc brettmc requested a review from a team as a code owner December 12, 2024 05:26
Copy link
Contributor

@Nevay Nevay left a comment

Choose a reason for hiding this comment

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

added some dummy/noop implementations of metrics components which are configurable but not implemented in our SDK

I'm not sure whether we should add not (yet) supported component providers (personally would prefer failing fast). Sadly the spec doesn't define the behavior yet ("TODO: define behavior if some portion of configuration model is not supported").


added noop support for xray and ottrace propagators (xray could be migrated into core from contrib?)

We could add a component provider with a PackageDependency on open-telemetry/contrib-aws, similar to other propagators that are also published in different packages (e.g. the B3 propagators).

@bobstrecansky bobstrecansky marked this pull request as draft December 18, 2024 13:04

public function defaultValue(mixed $value): static
{
$this->validate()->ifNull()->then(static function () use ($value): mixed {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is where I implemented "null/empty values with a default should use the default"

@brettmc brettmc marked this pull request as ready for review December 19, 2024 04:56
@brettmc
Copy link
Collaborator Author

brettmc commented Dec 23, 2024

@open-telemetry/php-approvers ready for review :)

@brettmc
Copy link
Collaborator Author

brettmc commented Dec 23, 2024

...just realised this is not compatible with php8.1 due to symfony/config. This might be a time to start talking about branching for a v2 release, which I've started a PR to work on.

@Nevay
Copy link
Contributor

Nevay commented Dec 24, 2024

Symfony ^5.4 and ^6.4 are compatible with PHP 8.1.
It should be possible to make the code compatible with the LTS releases while keeping compatibility with Symfony ^7.1 / the psalm errors seem easily fixable.

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.

2 participants