AWS X-Ray Remote Sampler Part 1 - Initial Classes and Rules Poller Implementation #3366
Conversation
|
Question: would it make sense to add this to https://github.com/open-telemetry/opentelemetry-python-contrib/tree/main/sdk-extension/opentelemetry-sdk-extension-aws instead of creating a new package? |
|
Yeah that makes sense, I synced with package owner @srprash, there is no concern with moving this Sampler to the SDK Extensions. |
...telemetry-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/sampler/_mock_clock.py
Outdated
Show resolved
Hide resolved
|
@xrmx looking for approval from maintainers, the PR has component owner approval. |
...k-extension-aws/src/opentelemetry/sdk/extension/aws/trace/sampler/aws_xray_remote_sampler.py
Outdated
Show resolved
Hide resolved
...k-extension-aws/src/opentelemetry/sdk/extension/aws/trace/sampler/aws_xray_remote_sampler.py
Outdated
Show resolved
Hide resolved
|
@xrmx Since this is a partial implementation, what is a good way to hide this new |
|
@xrmx @srprash Since the sampler is currently partially implemented, I've renamed the 2 X-Ray Sampler classes to be marked as internal for now via the following changes:
When the implementation is complete, I will rename |
|
|
||
| .. code-block:: python | ||
|
|
||
| from opentelemetry.sdk.extension.aws.trace.sampler import AwsXRayRemoteSampler |
There was a problem hiding this comment.
| from opentelemetry.sdk.extension.aws.trace.sampler import AwsXRayRemoteSampler | |
| from opentelemetry.sdk.extension.aws.trace.sampler import AwsXRayRemoteSampler |
Isn't this _-prefix though? If you don't want people to use it yet I won't add documentation yet
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| from typing import Optional |
There was a problem hiding this comment.
Could you please import from __future__ import annotations and use Foo | None = None instead?
| polling_interval = DEFAULT_RULES_POLLING_INTERVAL_SECONDS | ||
|
|
||
| self.__client_id = self.__generate_client_id() # pylint: disable=W0238 | ||
| self._clock = _Clock() |
| @@ -36,9 +36,8 @@ components: | |||
| - NathanielRN | |||
|
|
|||
| sdk-extension/opentelemetry-sdk-extension-aws: | |||
| - NathanielRN | |||
There was a problem hiding this comment.
Can someone from AWS explicitly ack this please?
There was a problem hiding this comment.
Thanks for the call out! @jj22ee is from AWS, so this is the right change 👍
| @@ -36,9 +36,8 @@ components: | |||
| - NathanielRN | |||
There was a problem hiding this comment.
@jj22ee you could also move the propagator under your name here too!
Description
This is an initial PR to address #3305 in order for OTel Python to support X-Ray Remote Sampling. A series of PRs to fully implement this feature will follow.
Changes:
opentelemetry-sdk-extension-awsType of change
Please delete options that are not relevant.
How Has This Been Tested?
This PR is not a full implementation of the sampler, so it cannot be tested for correct functionality.
Right now, it currently always return a Parent Based decision or a DROP decision as a placeholder.
A couple more PRs will follow-up to complete the implementation, which should look like this Sampler code from the AWS ADOT repo. At that point in the future, a full E2E integration test will be run.
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.