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

[Fix] Use service parameter to match centralized sampling rules #353

Merged
merged 1 commit into from
Sep 9, 2022

Conversation

srprash
Copy link
Contributor

@srprash srprash commented Sep 9, 2022

Issue #: #352

Description of changes:

  • Populate the service name in the sampling_req dictionary so that it can be matched with the centralized sampling rule configured for the service.

Testing:

  • Added a unit test test_begin_segment_matches_sampling_rule_on_name to verify the behavior.
    • Unit test result without the fix:

      ============================= test session starts ==============================
      collecting ... collected 1 item
      
      tests/test_recorder.py::test_begin_segment_matches_sampling_rule_on_name FAILED [100%]
      tests/test_recorder.py:315 (test_begin_segment_matches_sampling_rule_on_name)
      'rule_a' != 'rule_b'
      
      Expected :'rule_b'
      Actual   :'rule_a'
      
    • Unit test result after the fix:

      ============================= test session starts ==============================
      collecting ... collected 1 item
      
      tests/test_recorder.py::test_begin_segment_matches_sampling_rule_on_name PASSED [100%]
      

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

Copy link
Contributor

@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.

Looks good to me!

@srprash srprash merged commit 5d99316 into aws:master Sep 9, 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.

2 participants