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.

Sign up for GitHub

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

Jump to bottom

Enabled custom sampler configuration via env vars #2972

Merged
merged 27 commits into from
Oct 29, 2022
Merged

Enabled custom sampler configuration via env vars #2972

merged 27 commits into from
Oct 29, 2022

Conversation

jeremydvoss
Copy link
Contributor

@jeremydvoss jeremydvoss commented Oct 10, 2022 •
edited
Loading

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #2746

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Unit tests

  • Testing that if the OTEL_TRACES_SAMPLER is misconfigured, it still defaults to the ParentBased sampler.
  • Testing that if the OTEL_TRACES_SAMPLER is set to a custom sampler, it initializes correctly.
  • Testing that if the OTEL_TRACES_SAMPLER is set to a custom ratio-based sampler, it initializes correctly.
  • Pre-existing Sampling configuration tests still pass.

Manual tests:

  • Custom Sampler used when entry point specified via environment variable
  • Custom TraceIdRatioBased Sampler is initialized with OTEL_TRACES_SAMPLER_ARG
  • If a valid entry point is specified but it points to a non-existing class, it still defaults to the ParentBased sampler.
  • Build in Sampler names still work for OTEL_TRACES_SAMPLER env var.
  • Build in Samplers can be pointed to by entry points.

Does This PR Require a Contrib Repo Change?

No.

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

  • The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/

  • The method interfaces of test/util have changed

  • Scripts in scripts/ that were copied over to the Contrib repo have changed

  • Configuration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in

    • pyproject.toml
    • isort.cfg
    • .flake8

  • When a new .github/CODEOWNER is added

  • Major changes to project information, such as in:

    • README.md
    • CONTRIBUTING.md

  • Yes. - Link to PR:

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@jeremydvoss jeremydvoss changed the title Enbaled custom sampler configration via env vars Enabled custom sampler configuration via env vars Oct 10, 2022
@jeremydvoss jeremydvoss force-pushed the jeremyvoss branch 2 times, most recently from 5748cb9 to 4bbfbd6 Compare October 11, 2022 02:13
@jeremydvoss jeremydvoss marked this pull request as ready for review October 11, 2022 23:04
@jeremydvoss jeremydvoss requested a review from a team October 11, 2022 23:04
@lzchen lzchen added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Oct 14, 2022
aabmass
aabmass approved these changes Oct 14, 2022
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Some comments but LGTM thanks for updating :)

opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/util/__init__.py Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py Outdated Show resolved Hide resolved
opentelemetry-sdk/tests/trace/test_trace.py Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py Outdated Show resolved Hide resolved
aabmass
aabmass approved these changes Oct 18, 2022
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

LGTM!

opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py Outdated Show resolved Hide resolved
@srikanthccv srikanthccv self-assigned this Oct 27, 2022
@srikanthccv srikanthccv merged commit 35ba257 into open-telemetry:main Oct 29, 2022
This was referenced Nov 2, 2022
Closed
Closed
jeremydvoss added a commit to jeremydvoss/opentelemetry-python that referenced this pull request Nov 2, 2022
@jeremydvoss
Revert "Enabled custom sampler configuration via env vars (open-telem…
c14add0 
…etry#2972)"

This reverts commit 35ba257.
@jeremydvoss jeremydvoss mentioned this pull request Nov 2, 2022
Closed
8 tasks
jeremydvoss added a commit to jeremydvoss/opentelemetry-python that referenced this pull request Nov 2, 2022
@jeremydvoss
Revert "Enabled custom sampler configuration via env vars (open-telem…
965a2fe 
…etry#2972)"

This reverts commit 35ba257.
@jeremydvoss jeremydvoss mentioned this pull request Nov 10, 2022
Open
@jeremydvoss jeremydvoss mentioned this pull request Jan 24, 2023
Closed
@jeremydvoss jeremydvoss mentioned this pull request May 16, 2023
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabmass aabmass aabmass approved these changes

@srikanthccv srikanthccv srikanthccv approved these changes

@lzchen lzchen lzchen approved these changes

Assignees

@srikanthccv srikanthccv

Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow of configuration of Sampler in auto-instrumentation
4 participants
@jeremydvoss @aabmass @lzchen @srikanthccv