feat(tracer): specify subsegment name when capturing class method #1092
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Josh Kellendonk <misterjoshua@users.noreply.github.com>
Thank you for taking a look at this and good spotting the mistakes in the docstrings, I have committed your suggestions :) |
ijemmy
previously approved these changes
Oct 6, 2022
Comment on lines
+12
to
+13
| const customMetadataValue = process.env.EXPECTED_CUSTOM_METADATA_VALUE ? JSON.parse(process.env.EXPECTED_CUSTOM_METADATA_VALUE) : { bar: 'baz' }; | ||
| const customResponseValue = process.env.EXPECTED_CUSTOM_RESPONSE_VALUE ? JSON.parse(process.env.EXPECTED_CUSTOM_RESPONSE_VALUE) : { foo: 'bar' }; |
There was a problem hiding this comment.
Question: Why switching away from ?? here?
There was a problem hiding this comment.
Parsing a non-JSON / undefined value throws an error rather than evaluate to false, so if the goal was to use the default value (after the ??) when one is not explicitly provided in the env, then this should be correct.
Happy to revert if you think it's not ok or I misunderstood the intent of the original expression.
There was a problem hiding this comment.
No. I just want to check that it's really a missing edge case.
…egment-names-for-captured-methods
ijemmy
approved these changes
Oct 11, 2022
flochaz
approved these changes
Oct 11, 2022
dreamorosi
deleted the
1084-feature-tracer-allow-users-to-specify-segment-names-for-captured-methods
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of your changes
Community member @misterjoshua suggested a feature in #1084 to allow customers to set a custom name for the subsegment generated by the
Tracer.captureMethodmethod.At the moment Tracer automatically sets the subsegment name to the same value of the name of the method. This is a sensible choice but in some cases, when using non descriptive method names, customers might want to set their own names.
This PR adds an optional parameter to the options for
captureMethodso that the custom name can be set. The PR also splits the type used for the options incaptureMethodandcaptureLambdaHandlerso that the new parameter is valid only for the former (the latter will always be called## index.${HANDLER NAME}for now).The PR also adds unit test cases as well as e2e test cases and does some light housekeeping around the e2e tests and docstrings related to this feature.
How to verify this change
Check out newly added unit test cases and new e2e test cases (results here).
Related issues, RFCs
Issue number: #1084
PR status
Is this ready for review?: YES
Is it a breaking change?: NO
Checklist
Breaking change checklist
N/A
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.