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

Providers do not check instrumentation scope attributes when returning cached loggers, tracers, or meters #3208

Closed
dbarker opened this issue Dec 16, 2024 · 1 comment · Fixed by #3214
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@dbarker
Copy link
Contributor

dbarker commented Dec 16, 2024

Describe your environment
main branch at 6ed0651

Background
After working with the otel python api/sdk a bit and reviewing the 1.40.0 OTEL spec it seems that the c++ implementation of the GetTracer, GetLogger, and GetMeter may not be fully meeting the spec with respect to these methods returning distinct instances based on all parameters including the attributes. Reading the spec I'd expect that calling GetTracer with different attributes would return different tracers but that is not the case.

Steps to reproduce
The following test illustrates the issue.

TEST(TracerProviderTest, DistinctTracers)
{    
    auto processor = opentelemetry::sdk::trace::SimpleSpanProcessorFactory::Create(std::move(opentelemetry::exporter::trace::OStreamSpanExporterFactory::Create()));
    auto provider = opentelemetry::sdk::trace::TracerProviderFactory::Create(std::move(processor));

    auto tracer_1 = provider->GetTracer("my_library", "1.0.0", "my_schema_url");
    auto tracer_2 = provider->GetTracer("my_library", "1.0.0", "my_schema_url", {{ "instance", "one" }});
    auto tracer_3 = provider->GetTracer("my_library", "1.0.0", "my_schema_url", {{ "instance", "two" }});
    
    ASSERT_NE(tracer_1, nullptr);
    ASSERT_NE(tracer_2, nullptr);
    ASSERT_NE(tracer_3, nullptr);

    EXPECT_NE(tracer_1.get(), tracer_2.get()) << "providers must return distinct tracers if there are any differences in parameters passed to GetTracer";
    EXPECT_NE(tracer_2.get(), tracer_3.get()) << "providers must check for any difference in attributes too";   
}

What is the expected behavior?
Test passes

What is the actual behavior?
Test fails

 [==========] Running 1 test from 1 test suite.
 [----------] Global test environment set-up.
 [----------] 1 test from TracerProviderTest
 [ RUN      ] TracerProviderTest.DistinctTracers
 Expected: (tracer_1) != (tracer_2), actual: 0x5995d6ce5b70 vs 0x5995d6ce5b70
 providers must return distinct tracers if there are any differences in parameters passed to GetTracer
 Expected: (tracer_2) != (tracer_3), actual: 0x5995d6ce5b70 vs 0x5995d6ce5b70
 providers must check for any difference in attributes too
 [  FAILED  ] TracerProviderTest.DistinctTracers (0 ms)
 [----------] 1 test from TracerProviderTest (0 ms total)
 [----------] Global test environment tear-down
 [==========] 1 test from 1 test suite ran. (0 ms total)
 [  PASSED  ] 0 tests.
 [  FAILED  ] 1 test, listed below:
 [  FAILED  ] TracerProviderTest.DistinctTracers

Additional context
The current 1.40.0 OTEL specification has language that seems to imply any difference in parameters passed to get_logger, get_tracer, and get_meter must result in a distinct instance.

The term identical applied to Tracers describes instances where all parameters are equal. The term distinct applied to Tracers describes instances where at least one parameter has a different value.

The term identical applied to Meters describes instances where all parameters are equal. The term distinct applied to Meters describes instances where at least one parameter has a different value.

The term identical applied to Loggers describes instances where all parameters are equal. The term distinct applied to Loggers describes instances where at least one parameter has a different value.

The InstrumentationScope::equal method doesn't account for attributes

@dbarker dbarker added the bug Something isn't working label Dec 16, 2024
@github-actions github-actions bot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Dec 16, 2024
@dbarker
Copy link
Contributor Author

dbarker commented Dec 16, 2024

If my interpretation of the spec is correct, I'd be happy to help with a PR.

@marcalff marcalff added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants