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

Support tracing via the ObservabilityPolicy #2004

Merged
merged 11 commits into from
May 29, 2024

Conversation

sjberman
Copy link
Contributor

@sjberman sjberman commented May 21, 2024

Problem: As a user, I want to be able to enable tracing for requests flowing through NGF to my backend applications, so I can understand and debug my requests.

Solution: Using the ObservabilityPolicy, an app dev can now enable and configure tracing for their Route(s). The policy will only be applied if the NginxProxy configuration containing the tracing collector endpoint has been defined and attached to the GatewayClass. The policy can be attached to one or more HTTP or GRPC Routes.

Updated span attributes from the NginxProxy to be applied at the location block context, otherwise they are overwritten.

Also added functional tests to ensure that the complete solution is working.

Testing: Add functional tests for tracing

Verify end to end tracing by creating a collector and sending trace data to it.

Test cases:

  1. one policy attached to one route
  2. one policy attached to multiple routes

Also did lots of manual testing

Closes #1828
Closes #1911
Closes #1788

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Release notes

If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.

Support tracing via the ObservabilityPolicy CRD.

@github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request helm-chart Relates to helm chart labels May 21, 2024
Copy link

codecov bot commented May 21, 2024

Codecov Report

Attention: Patch coverage is 92.50936% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 87.61%. Comparing base (da36700) to head (ac840c4).

Files Patch % Lines
internal/mode/static/policies/policy.go 0.00% 16 Missing ⚠️
internal/mode/static/manager.go 20.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2004      +/-   ##
==========================================
- Coverage   87.66%   87.61%   -0.05%     
==========================================
  Files          93       96       +3     
  Lines        6557     6694     +137     
  Branches       50       50              
==========================================
+ Hits         5748     5865     +117     
- Misses        753      773      +20     
  Partials       56       56              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sjberman sjberman marked this pull request as ready for review May 21, 2024 19:33
@sjberman sjberman requested review from a team as code owners May 21, 2024 19:33
internal/mode/static/policies/observability/generator.go Outdated Show resolved Hide resolved
internal/mode/static/policies/observability/validator.go Outdated Show resolved Hide resolved
internal/mode/static/policies/observability/validator.go Outdated Show resolved Hide resolved
tests/suite/tracing_test.go Outdated Show resolved Hide resolved
tests/suite/tracing_test.go Outdated Show resolved Hide resolved
tests/suite/tracing_test.go Show resolved Hide resolved
tests/suite/tracing_test.go Outdated Show resolved Hide resolved
tests/framework/request.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

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

@sjberman can you add some tests here:

NOTE: When adding a new NGF policy to the change processor,

We just need to make sure the shared policy store correctly handles multiple policy types.

tests/framework/request.go Outdated Show resolved Hide resolved
tests/suite/telemetry_test.go Outdated Show resolved Hide resolved
tests/suite/tracing_test.go Show resolved Hide resolved
tests/suite/tracing_test.go Outdated Show resolved Hide resolved
tests/suite/tracing_test.go Show resolved Hide resolved
tests/suite/tracing_test.go Outdated Show resolved Hide resolved
@sjberman sjberman requested review from pleshakov and kate-osborn May 28, 2024 18:29
internal/mode/static/state/dataplane/configuration.go Outdated Show resolved Hide resolved
internal/mode/static/state/dataplane/configuration.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/graph.go Show resolved Hide resolved
internal/mode/static/state/graph/nginxproxy.go Outdated Show resolved Hide resolved
tests/suite/tracing_test.go Outdated Show resolved Hide resolved
@sjberman sjberman requested a review from pleshakov May 29, 2024 16:37
internal/mode/static/policies/policy.go Outdated Show resolved Hide resolved
internal/mode/static/policies/policy.go Show resolved Hide resolved
internal/mode/static/state/graph/graph.go Outdated Show resolved Hide resolved
tests/suite/tracing_test.go Show resolved Hide resolved
sjberman added 8 commits May 29, 2024 12:24
Problem: As a user, I want to be able to enable tracing for requests flowing through NGF to my backend applications, so I can understand and debug my requests.

Solution: Using the ObservabilityPolicy, an app dev can now enable and configure tracing for their Route(s). The policy will only be applied if the NginxProxy configuration containing the tracing collector endpoint has been defined and attached to the GatewayClass. The policy can be attached to one or more HTTP or GRPC Routes.

Updated span attributes from the NginxProxy to be applied at the location block context, otherwise they are overwritten.

Also added functional tests to ensure that the complete solution is working.
Verify end to end tracing by creating a collector and sending trace data to it.

Test cases:
1. one policy attached to one route
2. one policy attached to multiple routes
@sjberman sjberman requested a review from kate-osborn May 29, 2024 18:51
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

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

🚀 🚀

@sjberman sjberman merged commit 4859cca into nginxinc:main May 29, 2024
40 checks passed
@sjberman sjberman deleted the enh/obs-policy branch May 29, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request helm-chart Relates to helm chart release-notes
Projects
Archived in project
3 participants