-
Notifications
You must be signed in to change notification settings - Fork 302
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
Added AWS SDK Changes to support Bedrock spans and Application Signals #2458
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2458 +/- ##
===========================================
+ Coverage 73.91% 84.20% +10.28%
===========================================
Files 267 34 -233
Lines 9615 747 -8868
===========================================
- Hits 7107 629 -6478
+ Misses 2508 118 -2390
|
Not sure why the bedrock sem cov test is failing for v1.28. I didn't change how the sem covs are set there. |
src/OpenTelemetry.Instrumentation.AWS/Implementation/AWSLlmModelProcessor.cs
Show resolved
Hide resolved
} | ||
catch (Exception ex) | ||
{ | ||
Console.WriteLine("Exception: " + ex.Message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
catch (Exception ex)
{
Console.WriteLine("Exception: " + ex.Message);
I forget, is this standard practice for exception handling within this library? I'm fine swallowing parsing exceptions - but I don't recall if there is a preferred channel to let the user know there was a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually i don't think there is any standard practice for handling exceptions in this specific lib. However, the standard convention throughout the contrib lib as whole is to use event sources like this for example: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/55121760115c94e11571057f4263c43f4d71ad21/src/OpenTelemetry.Resources.AWS/AWSResourcesEventSource.cs. I'll add one for the Opentelemetry.Instrumentation.AWS
/// <summary> | ||
/// Not yet incorporated in Semantic Conventions repository. Check if more details need to be added | ||
/// </summary> | ||
public virtual string AttributeGenAiTopP => string.Empty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is defined in the semantic convention here: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/gen-ai/gen-ai-spans.md?plain=1#L60
@@ -74,6 +84,12 @@ private abstract class AWSSemanticConventionsLegacy : AWSSemanticConventionsBase | |||
// GEN AI Attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the GenAI documentation page, it looks like the new GenAI attributes all existed prior to v1.27: https://github.com/open-telemetry/semantic-conventions/blob/v1.27.0/docs/gen-ai/gen-ai-spans.md
Is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if they existed prior to v1.27 but they're in v1.27 which means they fall into the legacy conventions.
public override string AttributeAWSSQSQueueName => "aws.sqs.queue_name"; | ||
public override string AttributeAWSS3BucketName => "aws.s3.bucket"; | ||
public override string AttributeAWSKinesisStreamName => "aws.kinesis.stream_name"; | ||
public override string AttributeAWSLambdaResourceMappingId => "aws.lambda.resource_mapping.id"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the attributes that are not yet part of the Semantic Convention - I'm not sure if they should be added as "Legacy".
I'm also not sure if they should be added to an existing code page - ie it feels odd to add them to say v.1.29.0 when they aren't actually part of that spec either.
Any thoughts on this?
OR - it might be better to first send a PR to the Semantic Convention repo and get the attributes added - then they'll be part of a SemConv release and we wont have this problem. Given they're attributes are all in the AWS namespace I would expect the PR to be accepted rather quickly.
7ab1a54
to
9297cd1
Compare
25825b1
to
7201785
Compare
7201785
to
3cc4599
Compare
3cc4599
to
012a012
Compare
012a012
to
ddd68f3
Compare
Changes
Added changes to detect bedrock spans and populate attributes accordingly. Also added logic to populate aws specific attributes used by AWS Application Signals like "aws.region", "aws.service", "aws.operation", "aws.requestId", etc. We are currently using our own fork of Opentelemetry-Instrumentation-AWS here: https://github.com/aws-observability/aws-otel-dotnet-instrumentation/tree/main/src/OpenTelemetry.Instrumentation.AWS. This change syncs our local fork so that we can stop relying on it and instead reference this directly instead.
I did some manual testing and was able to verify that the spans created contain the required information.
Pushing this PR for now to get some review going while we add some unit tests for the Bedrock related changes. Will add changes to
CHANGELOG.md
. No changes to public API.Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes