feat(opentelemetry-sampler-aws-xray): add rule cache and rule matching #1463
Conversation
carolabadeer
force-pushed
the
xray-remote-sampling-2
branch
from
ba74be4 to
fdf9f7d
Compare
carolabadeer
changed the title
packages/opentelemetry-sampler-aws-xray/src/fallback-sampler.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Sorry for taking so long to review.
Added comments, mostly regarding performance, style, and conventions.
Since this package is hosted in opentelemetry but is written and maintained by AWS engineers, please feel free to resolve any thread that you are not interested in implementing or do not agree with.
packages/opentelemetry-sampler-aws-xray/src/fallback-sampler.ts
Outdated
Show resolved
Hide resolved
| .match(convertPatternToRegex(pattern.toLowerCase())); | ||
|
|
||
| if (match === null) { | ||
| console.log(`WildcardMatch: no match found for ${text}`); |
There was a problem hiding this comment.
I know this is not the final artifact, but recommend using diag for these, as console.log can be easily forgotten in the future and print into end user apps.
There was a problem hiding this comment.
Definitely - since this feedback was given in the first PR, it will be incorporated in all 3 as well
|
|
||
| const match = text | ||
| .toLowerCase() | ||
| .match(convertPatternToRegex(pattern.toLowerCase())); |
There was a problem hiding this comment.
I saw this package in npm and wondering if we can replace some of the code here with it.
This will reduce code we need to maintain test and understand while benefiting from a mature package that abstract away many of the nitty details.
Not sure though if it's compatible to the format that x-ray uses, but that should be easy to check.
| resource.attributes[SemanticResourceAttributes.CLOUD_PLATFORM]; | ||
|
|
||
| // "Default" is a reserved keyword from X-Ray back-end. | ||
| if (this.RuleName === 'Default') { |
There was a problem hiding this comment.
Since it's used twice in the code (also in rule-cache.ts), consider extracting this const to consts.ts and copying the comment there.
|
@carolabadeer Is this PR something you are still working on, or do you know if there is someone else at AWS who might be planning to pick this back up? It's a bit of a rough spot having part of the package already in the repo, though we have it in a incubating directory separate from the other published packages. |
|
Hi @JamieDanielson, someone from AWS will likely be picking this up, although I am not sure about the timeline. |
Okay thanks for confirming! For now we are working on reviewing old PRs and think it makes sense to close this for now as it's not actively being worked on. When someone is ready to pick it back up, feel free to reopen this or open a new PR with these changes. |
Which problem is this PR solving?
Short description of the changes
Temporary note: Since the first PR that this builds on has not yet been merged to main, the commits in it persist in this PR. Please only review the "add rule cache and rule matching" commit onwards, as the previous commits will be resolved once changes from main are pulled in.