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

Removing automatic service name from Lambda instrumentation #1080

Merged

Conversation

ehornby
Copy link
Contributor

@ehornby ehornby commented Mar 14, 2023

Changes

Adding a ServiceNameOverride property to AwsLambdaInstrumentationOptions for instances where desired service name is different than Lambda function name.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 14, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: ehornby / name: Eric Hornby (bb7b7a0)

@ehornby ehornby marked this pull request as ready for review March 14, 2023 19:57
@ehornby ehornby requested a review from a team March 14, 2023 19:57
@utpilla utpilla added the comp:instrumentation.awslambda Things related to OpenTelemetry.Instrumentation.AWSLambda label Mar 14, 2023
@codecov
Copy link

codecov bot commented Mar 14, 2023

Codecov Report

Merging #1080 (f87fec2) into main (6c29bc9) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head f87fec2 differs from pull request most recent head 2df4446. Consider uploading reports for the commit 2df4446 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1080      +/-   ##
==========================================
- Coverage   69.31%   69.26%   -0.05%     
==========================================
  Files         214      212       -2     
  Lines        8358     8337      -21     
==========================================
- Hits         5793     5775      -18     
+ Misses       2565     2562       -3     
Impacted Files Coverage Δ
...ation.AWSLambda/AWSLambdaInstrumentationOptions.cs 100.00% <100.00%> (ø)
...ation.AWSLambda/TracerProviderBuilderExtensions.cs 100.00% <100.00%> (ø)

... and 13 files with indirect coverage changes

@Kielek
Copy link
Contributor

Kielek commented Mar 15, 2023

@Oberon00, could you please review?
@ehornby, if the changes will be approved, we need record in CHANGELOG.md.

@@ -55,7 +55,11 @@ public static class TracerProviderBuilderExtensions
builder.AddSource(AWSLambdaWrapper.ActivitySourceName);
builder.SetResourceBuilder(ResourceBuilder
Copy link
Member

@Oberon00 Oberon00 Mar 15, 2023

Choose a reason for hiding this comment

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

I don't think this is necessary or even very useful: Using the new ConfigureResource API introduced in open-telemetry/opentelemetry-dotnet#2909 + open-telemetry/opentelemetry-dotnet#3307, one can override the service more easily already.

But: It would be good if this call here could also be changed to use ConfigureResource instead of SetResourceBuilder, so it does not reset any other resources set before it. This is only tangentially related though and not a prerequisite for overriding the service afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Oberon00 Completely agree - there are fine existing ways to override the service name as is, I'm currently using the ConfigureResource API in existing implementations.

Where I saw value here was in the lack of clarity that such an override is necessary in the first place, if one required their service name to differ from the Lambda function name. Documentation indicates that setting the OTEL_SERVICE_NAME environment variable (or injecting attributes via OTEL_RESOURCE_ATTRIBUTES) should set the service.name attribute as desired, but that didn't seem to be what happened in practice.

I didn't find it immediately clear anywhere that behavior is different with Lambdas using this package, so this change wasn't intended to add functionality so much as just clarity.

Copy link
Member

@Oberon00 Oberon00 Mar 15, 2023

Choose a reason for hiding this comment

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

This is currently still in beta, so we may break things if we find we need to. I think that this overrides an explicitly set OTEL_SERVICE_NAME is a bug. IMHO, it would be good to completely remove setting the service name here, and we can maybe later add it back under a dedicated off-by-default option (or even just provide an API that returns the function name for users to set as service name themselves)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It certainly seems more like a bug than intended behaviour, for sure.

I'm happy to repurpose this PR to remove the functionality that sets the service name, if you'd like?

Copy link
Member

Choose a reason for hiding this comment

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

I think that would be a better solution for the problem you describe. Thank you!

@ehornby ehornby closed this Mar 17, 2023
@ehornby ehornby deleted the awsLambdaWrapper_addServiceNameOverride branch March 17, 2023 17:25
@ehornby ehornby restored the awsLambdaWrapper_addServiceNameOverride branch March 17, 2023 17:27
@ehornby ehornby reopened this Mar 17, 2023
@ehornby ehornby changed the title adding override to instrumentation options Removing automatic service name from Lambda instrumentation Mar 21, 2023
@Oberon00
Copy link
Member

Oberon00 commented Mar 21, 2023

Please add a CHANGELOG entry and also check if the README or any xmldoc needs to be updated, then this is good to go from my perspective. Thank you!

Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

LGTM, per discussion in this PR.

@Kielek Kielek merged commit d293201 into open-telemetry:main Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.awslambda Things related to OpenTelemetry.Instrumentation.AWSLambda
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants