Skip to content

Eng 9536 add filters #223

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

Closed
wants to merge 7 commits into from
Closed

Eng 9536 add filters #223

wants to merge 7 commits into from

Conversation

prodion23
Copy link
Collaborator

Description

This PR adds filter support for flask & grpc instrumentation.

Testing

Tests added here mimic the behavior of existing grpc & flask tests. & assert the permission denied & 403 errors.

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

@prodion23 prodion23 requested review from jcchavezs and davexroth July 6, 2021 21:58
Copy link
Contributor

@rcbjBlueMars rcbjBlueMars left a comment

Choose a reason for hiding this comment

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

The tox.ini files in the tests/flask and tests/grpc directories need to be updated to reference these new test cases.

tox.ini Outdated
commands=pytest -rPx src/hypertrace
commands=
pytest -rPx src/hypertrace
pytest -rPx tests/agent
Copy link
Contributor

Choose a reason for hiding this comment

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

New integration tests should be added in the Makefile's integration-tests rule, not here.

Copy link
Collaborator Author

@prodion23 prodion23 Jul 7, 2021

Choose a reason for hiding this comment

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

Okay, just to verify, unit tests are expected to be in the src directory alongside library files?
Integrations tests belong under tests/<framework>?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is the pattern @jcchavezs established.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Awesome, thanks for the clarification here :)

@prodion23 prodion23 requested a review from rcbjBlueMars July 7, 2021 14:53
@prodion23 prodion23 closed this Jul 7, 2021
@prodion23 prodion23 deleted the ENG-9536-add-filters branch July 26, 2021 15:04
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