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 of built-in samplers in SDK configuration #3176

Merged
merged 7 commits into from
Mar 15, 2023

Conversation

matej-g
Copy link
Contributor

@matej-g matej-g commented Feb 15, 2023

Description

Fixes a configuration bug which causes the instrumentation to fall back on the default sampler when a built-in sampler is specified, due to the samplers missing from entry points. For details see description of #3175.

P.S. I'm not a Python developer, so feedback on if this should be done other way is welcome.

Fixes #3175

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?

Does This PR Require a Contrib Repo Change?

  • 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

@matej-g matej-g changed the title Fix use of in-built samplers in SDK configuration Fix use of built-in samplers in SDK configuration Feb 15, 2023
@matej-g matej-g marked this pull request as ready for review February 15, 2023 15:34
@matej-g matej-g requested a review from a team February 15, 2023 15:34
ShauliSolomovich
ShauliSolomovich approved these changes Feb 15, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
opentelemetry-sdk/pyproject.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Added a question

Signed-off-by: Matej Gera <matej.gera@coralogix.com>
Signed-off-by: Matej Gera <matej.gera@coralogix.com>
Signed-off-by: Matej Gera <matej.gera@coralogix.com>
@matej-g
Copy link
Contributor Author

matej-g commented Mar 10, 2023

Hi @srikanthccv, sorry for the back and forth, I implemented it with your suggestion and cleaned up the PR. Hopefully now all is as should be.

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

Thanks @matej-g , but the type conversion is also missing here

sampler_arg = os.getenv(OTEL_TRACES_SAMPLER_ARG, "")
sampler = sampler_factory(sampler_arg)
This is an existing bug, let me know if you want to make the change in this PR otherwise I will get it fixed separately and then merge this.

It should be changed to

sampler_factory = _import_sampler_factory(sampler_name)

kwargs = {}
if sampler_name in ("traceidratio", "parentbased_traceidratio"):
    try:
        rate = float(os.getenv(OTEL_TRACES_SAMPLER_ARG))
    except ValueError:
        _logger.warning("Could not convert TRACES_SAMPLER_ARG to float.")
        rate = 1.0
    kwargs["rate"] = rate

sampler = sampler_factory(**kwargs)

Signed-off-by: Matej Gera <matej.gera@coralogix.com>
Signed-off-by: Matej Gera <matej.gera@coralogix.com>
@matej-g
Copy link
Contributor Author

matej-g commented Mar 15, 2023

@srikanthccv you're right, that part was still missing. I added your suggestion. Hopefully now everything is fixed as expected.

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

Thank you

@ocelotl ocelotl merged commit a70e4a0 into open-telemetry:main Mar 15, 2023
@dnshio
Copy link

dnshio commented Mar 16, 2023

Hi. Thanks for this fix. Do you know when this fix will be available on PyPI?

@srikanthccv
Copy link
Member

most probably (early) next week based on our monthly release schedule.

@dnshio
Copy link

dnshio commented Mar 16, 2023

most probably (early) next week based on our monthly release schedule.

Thanks @srikanthccv 🙌🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants