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

AWS X-Ray Remote Sampler Part 1 - Initial Rules Poller Implementation #33

Merged
merged 10 commits into from
Feb 7, 2024

Conversation

jj22ee
Copy link
Contributor

@jj22ee jj22ee commented Jan 30, 2024

Issue #, if available:
First PR of 3 parts for adding the X-Ray remote sampling support for OTel Python SDK.

Description of changes:

  • Python Classes
    • AwsXRayRemoteSampler - extends opentelemetry.sdk.trace.sampling.Sampler and implements should_sample.
      • Upon initialization, starts polling for sampling rules by scheduling a threading.Timer to execute a poll after a configurable interval of time. After this interval, it will repeat this process indefinitely by scheduling the same threading.Timer upon completion of the previous timer.
      • OTel resource, Collector endpoint, rules polling_interval are configurable.
    • AwsXRaySamplingClient - client to call GetSamplingRules
    • SamplingRule - Class for SamplingRules type

Testing Script to poll Sampling Rules every 5 seconds:

import logging
import time

from amazon.opentelemetry.distro.sampler.aws_xray_remote_sampler import AwsXRayRemoteSampler
from opentelemetry.sdk.resources import Resource

logging.basicConfig(level=logging.INFO)
sampler = AwsXRayRemoteSampler(Resource.get_empty(), polling_interval=5)

time.sleep(15)

Output:

88665a53c0dd:sampler jjllee$ python3 mytesting.py 
INFO:amazon.opentelemetry.distro.sampler.aws_xray_remote_sampler:Got Sampling Rules: {'[{"Attributes": {}, "FixedRate": 0.05, "HTTPMethod": "*", "Host": "*", "Priority": 10000, "ReservoirSize": 100, "ResourceARN": "*", "RuleARN": "arn:aws:xray:us-east-1:999999999999:sampling-rule/Default", "RuleName": "Default", "ServiceName": "*", "ServiceType": "*", "URLPath": "*", "Version": 1}, {"Attributes": {"abc": "1234"}, "FixedRate": 0.11, "HTTPMethod": "*", "Host": "*", "Priority": 20, "ReservoirSize": 1, "ResourceARN": "*", "RuleARN": "arn:aws:xray:us-east-1:999999999999:sampling-rule/test", "RuleName": "test", "ServiceName": "*", "ServiceType": "*", "URLPath": "*", "Version": 1}]'}
INFO:amazon.opentelemetry.distro.sampler.aws_xray_remote_sampler:Got Sampling Rules: {'[{"Attributes": {}, "FixedRate": 0.05, "HTTPMethod": "*", "Host": "*", "Priority": 10000, "ReservoirSize": 100, "ResourceARN": "*", "RuleARN": "arn:aws:xray:us-east-1:999999999999:sampling-rule/Default", "RuleName": "Default", "ServiceName": "*", "ServiceType": "*", "URLPath": "*", "Version": 1}, {"Attributes": {"abc": "1234"}, "FixedRate": 0.11, "HTTPMethod": "*", "Host": "*", "Priority": 20, "ReservoirSize": 1, "ResourceARN": "*", "RuleARN": "arn:aws:xray:us-east-1:999999999999:sampling-rule/test", "RuleName": "test", "ServiceName": "*", "ServiceType": "*", "URLPath": "*", "Version": 1}]'}
INFO:amazon.opentelemetry.distro.sampler.aws_xray_remote_sampler:Got Sampling Rules: {'[{"Attributes": {}, "FixedRate": 0.05, "HTTPMethod": "*", "Host": "*", "Priority": 10000, "ReservoirSize": 100, "ResourceARN": "*", "RuleARN": "arn:aws:xray:us-east-1:999999999999:sampling-rule/Default", "RuleName": "Default", "ServiceName": "*", "ServiceType": "*", "URLPath": "*", "Version": 1}, {"Attributes": {"abc": "1234"}, "FixedRate": 0.11, "HTTPMethod": "*", "Host": "*", "Priority": 20, "ReservoirSize": 1, "ResourceARN": "*", "RuleARN": "arn:aws:xray:us-east-1:999999999999:sampling-rule/test", "RuleName": "test", "ServiceName": "*", "ServiceType": "*", "URLPath": "*", "Version": 1}]'}
88665a53c0dd:sampler jjllee$ 

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jj22ee jj22ee requested a review from a team January 30, 2024 21:02
@jj22ee jj22ee requested review from AsakerMohd and srprash January 30, 2024 22:25
@srprash
Copy link
Contributor

srprash commented Feb 1, 2024

In the PR description can you also post the output from your testing script?

@srprash
Copy link
Contributor

srprash commented Feb 1, 2024

Also, I didn't go through the unit tests in this pass. Will review them some time soon.

@jj22ee
Copy link
Contributor Author

jj22ee commented Feb 6, 2024

Pending #36 to fix failing checks

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.

Minor comments regarding the types of SamplingRule properties. Since typing in python is very fluid, it can easily lead to confusion and possible bugs if we don't treat the properties with correct types.

@jj22ee jj22ee merged commit 543792f into main Feb 7, 2024
14 checks passed
@jj22ee jj22ee deleted the remote-sampler branch February 7, 2024 01:22
jj22ee added a commit that referenced this pull request Feb 14, 2024
…g Logic (#47)

*Issue #, if available:*
Second PR of 3 parts for adding the X-Ray remote sampling support for
OTel Python SDK.
[See Part
1](#33)

*Description of changes:*

- Sampling `RuleCache`
- Caches a list of `Rule`s, ordered by rule priority then rule name.
Each rule corresponds to the Sampling Rule from GetSamplingRules. Each
call to GetSamplingRules will only update the `Rule`s that have changed
properties, to preserve the state of unchanged rules. This means when
Reservoir and Statistics are implemented in the `Rule`s, they will
persist for unchanged rules.
- The RuleCache will determine which Rule a set of
{ResourceAttributes,SpanAttributes} matches with that has highest
priority.
- `Rule`
- Corresponds to a `SamplingRule` and has logic to match with provided
set of ResourceAttribute and SpanAttribute using the `Matcher` class.
  - Will determine the final sampling decision
- `Matcher` class with methods to perform:
  - Convert X-Ray sampling rule options to regex patterns
  - Wild card and attribute matching
- Initial class for `FallbackSampler`

Testing:

Unit tests

By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice.
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