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.

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

Update README.md #363

Merged
merged 6 commits into from
Nov 21, 2022
Merged

Update README.md #363

merged 6 commits into from
Nov 21, 2022

Conversation

atshaw43
Copy link
Contributor

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@atshaw43 atshaw43 requested a review from a team as a code owner November 17, 2022 17:06
@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2022

Codecov Report

Merging #363 (29d6c11) into master (3591822) will increase coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #363      +/-   ##
============================================
+ Coverage     58.39%   58.41%   +0.01%     
- Complexity     1228     1229       +1     
============================================
  Files           133      133              
  Lines          5011     5011              
  Branches        589      589              
============================================
+ Hits           2926     2927       +1     
  Misses         1814     1814              
+ Partials        271      270       -1     
Impacted Files Coverage Δ
...ava/com/amazonaws/xray/AWSXRayRecorderBuilder.java 69.29% <0.00%> (+0.87%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

README.md Outdated
boolean toSample = false;
for (SQSMessage message: event.getRecords()) {
if (SQSMessageHelper.isSampled(message)) {
toSample = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this always sample the subsegment even when a single message in the batch is sampled?
I thought how oversampling mitigation would work is that when processing each message in the batch, we decided whether to begin a subsegment with/without sampling based on if the message is sampled or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to demonstrate how to do a fan-in.

README.md Outdated
Comment on lines 252 to 264
if (toSample) {
AWSXRay.beginSubsegment("Processing Message");
} else {
AWSXRay.beginSubsegmentWithoutSampling("Processing Message");
}

// Do your procesing work here
System.out.println("Doing processing work");

// End your subsegment
AWSXRay.endSubsegment();

return "Success";
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are processing each message in the batch, shouldn't this code be inside the for loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment above.

README.md Outdated
@@ -228,6 +228,43 @@ try {
```
Note that in the closure-based example above, exceptions are intercepted automatically.

### Oversampling Mitigation
Oversampling mitigation allows you to ignore a parent segment/subsegment's sampled flag and instead set it to false.
The code below demonstrates overriding the sampled flag based on the SQS messages sent to Lambda.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be reworded to explain what is happening a little more clearly.

"The code below demonstrates creating a sampled or un-sampled subsegment based on the sampling status of each SQS message processed by Lambda"
^^ How does something like this sound?

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to show how to do a fan-in. I don't think the rewording reflects that.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Contributor

@srprash srprash left a comment

Choose a reason for hiding this comment

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

LGTM!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
README.md Outdated
@@ -228,6 +228,70 @@ try {
```
Note that in the closure-based example above, exceptions are intercepted automatically.

### Oversampling Mitigation
Oversampling mitigation allows you to ignore a parent segment/subsegment's sampled flag and instead set it to false.

Choose a reason for hiding this comment

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

I think this sentence still needs some rewording - it is unclear what "it" is referring to here

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link

@carolabadeer carolabadeer left a comment

Choose a reason for hiding this comment

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

LGTM

@atshaw43 atshaw43 merged commit 1311d58 into master Nov 21, 2022
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.

None yet

4 participants