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

feat: Respect "suppress_instrumentation" in redis instrumentor #2064

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

martimors
Copy link

@martimors martimors commented Nov 15, 2023

Description

Fixes #2057

I'll wait until I get some feedback to update the changelog and the docs if need be.

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

  • Manual test
  • Unit test

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

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

Copy link

linux-foundation-easycla bot commented Nov 15, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@@ -182,6 +183,11 @@ def _traced_execute_command(func, instance, args, kwargs):
query = _format_command_args(args)
name = _build_span_name(instance, args)

if context.get_value(
_SUPPRESS_INSTRUMENTATION_KEY
):
Copy link
Member

Choose a reason for hiding this comment

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

There is a function called is_instrumentation_enabled in the opentelemetry.instrumentation.utils module that should be used instead of accessing _SUPRESS_INSTRUMENTATION_KEY directly.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I've addressed this. I'd like to run the tests but I can't figure out how to do so, the tox command just goes into an error loop (MacOS). Can you help?

Copy link
Member

Choose a reason for hiding this comment

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

Without knowing what arguments you are passing and what error you are getting it's hard to give any specific suggestion.

Copy link
Author

Choose a reason for hiding this comment

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

No arguments, #2330

@martimors
Copy link
Author

Sorry for the commit spam - I'm just reviewing myself as best I can while not being able to run the tests locally. Do you have some guide how to get that up and running, preferably in the IDE and not tox on the cli? That would make this development experience a lot better for me.

@martimors
Copy link
Author

@federicobond are we good to go here?

@ocelotl
Copy link
Contributor

ocelotl commented Apr 22, 2024

@martimors please take a look at the failures. The only way we can run tests is by using tox, which actually will create a virtual environment with all the testing dependencies installed and the pytest will run the tests.

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.

RedisInstrumentor doesn't respect "supress_instrumentation" flag
5 participants