Skip to content

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Sep 18, 2025

@trentm trentm self-assigned this Sep 18, 2025
Copy link

codecov bot commented Sep 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.67%. Comparing base (4e49448) to head (c30d403).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3076      +/-   ##
==========================================
- Coverage   92.67%   92.67%   -0.01%     
==========================================
  Files         237      237              
  Lines       11247    11246       -1     
  Branches     2334     2333       -1     
==========================================
- Hits        10423    10422       -1     
  Misses        824      824              
Flag Coverage Δ
sampler-aws-xray 96.30% <ø> (-0.01%) ⬇️

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

Files with missing lines Coverage Δ
...ages/sampler-aws-xray/src/sampling-rule-applier.ts 97.24% <ø> (-0.03%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines 42 to 43
// XXX can we drop this? (else replace it with semconv-obsolete.ts usage)
SEMRESATTRS_FAAS_ID,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jj22ee Hi. I have a question about the faas.id usage here.

Background: I've been working on removing all of the usages of the old/deprecated exports from the @opentelemetry/semantic-conventions package, which includes all of the SEMRESATTRS_* exports -- as part of #2377.
This usage is one of the last ones.

faas.id was replaced with cloud.resource_id in SemConv 1.19 in 2023: https://github.com/open-telemetry/semantic-conventions/blob/main/CHANGELOG.md#v1190-2023-03-06

What is an expected case where getLambdaArn() (below) will encounter a resource with the faas.id attribute? I'm not challenging, just trying to understand how this sampler would be used.

instrumentation-aws-lambda does still use the old faas.id (

const span = plugin.tracer.startSpan(
name,
{
kind: SpanKind.SERVER,
attributes: {
[ATTR_FAAS_EXECUTION]: context.awsRequestId,
[ATTR_FAAS_ID]: context.invokedFunctionArn,
) though it sets taht as a span attribute and not as a resource attribute.

Copy link
Contributor

@jj22ee jj22ee Sep 19, 2025

Choose a reason for hiding this comment

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

TLDR: SEMRESATTRS_FAAS_ID can be dropped, doesn't change the experience for users of X-Ray Sampler.


Today there isn't an expected case where getLambdaArn() would encounter a resource with faas.id because lambda instrumentation or resource-detector doesn't set it, and even if they did, it doesn't matter for X-Ray Sampler today because:

So effectively, the result of getLambdaArn() doesn't change anything, as all sampling rules will match its result because of the * wildcard match. The check for faas.id in getLambdaArn() was intended for future-proofing in case X-Ray does want to support letting users match Sampling Rule's Resource ARN to getLambdaArn().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dropped in commit 9d0f303

@trentm trentm marked this pull request as ready for review September 22, 2025 17:47
@trentm trentm requested a review from a team as a code owner September 22, 2025 17:47
@trentm trentm enabled auto-merge (squash) September 23, 2025 19:07
@trentm trentm merged commit 09d57b4 into open-telemetry:main Sep 23, 2025
16 checks passed
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.

4 participants