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.

Sign up for GitHub

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
merged 14 commits into from
Mar 22, 2023
Merged

Removing automatic service name from Lambda instrumentation #1080

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
bb7b7a0
adding override to instrumentation options
ehornby Mar 14, 2023
f87fec2
Merge branch 'main' into awsLambdaWrapper_addServiceNameOverride
ehornby Mar 14, 2023
08301e6
Merge branch 'main' into awsLambdaWrapper_addServiceNameOverride
ehornby Mar 15, 2023
e8cd93a
Merge branch 'main' into awsLambdaWrapper_addServiceNameOverride
ehornby Mar 16, 2023
2766f02
Merge branch 'main' into awsLambdaWrapper_addServiceNameOverride
ehornby Mar 17, 2023
44c9ad6
rolling back initial changes
ehornby Mar 17, 2023
8efc1e4
Merge branch 'awsLambdaWrapper_addServiceNameOverride' of https://git…
ehornby Mar 17, 2023
489ab0e
Merge branch 'main' into awsLambdaWrapper_addServiceNameOverride
ehornby Mar 21, 2023
0a8dd4d
updating changelog
ehornby Mar 22, 2023
6a6e23c
Merge branch 'awsLambdaWrapper_addServiceNameOverride' of https://git…
ehornby Mar 22, 2023
b099a75
Update src/OpenTelemetry.Instrumentation.AWSLambda/CHANGELOG.md
ehornby Mar 22, 2023
8ca03ca
remove trailing space
ehornby Mar 22, 2023
ad6af5e
remove trailing space
ehornby Mar 22, 2023
2df4446
Merge branch 'main' into awsLambdaWrapper_addServiceNameOverride
ehornby Mar 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/OpenTelemetry.Instrumentation.AWSLambda/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
([#626](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/626))
* Removes `AddAWSLambdaConfigurations` method with default configure parameter.
([#943](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/943))
* BREAKING (behavior): `AddAWSLambdaConfigurations` no longer calls `AddService`
([#1080](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1080))

## 1.1.0-beta.2

Expand Down
1 change: 0 additions & 1 deletion src/OpenTelemetry.Instrumentation.AWSLambda/TracerProviderBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ public static TracerProviderBuilder AddAWSLambdaConfigurations(
builder.AddSource(AWSLambdaWrapper.ActivitySourceName);
builder.SetResourceBuilder(ResourceBuilder
Copy link
Member

@Oberon00 Oberon00 Mar 15, 2023 •
edited
Loading

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 •
edited
Loading

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!

.CreateEmpty()
.AddService(AWSLambdaUtils.GetFunctionName(), null, null, false)
.AddTelemetrySdk()
.AddAttributes(AWSLambdaResourceDetector.Detect()));

Expand Down