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

[TT-7902] Detailed Tracing #5294

Merged
merged 26 commits into from
Jul 31, 2023
Merged

[TT-7902] Detailed Tracing #5294

merged 26 commits into from
Jul 31, 2023

Conversation

mativm02
Copy link
Contributor

@mativm02 mativm02 commented Jul 7, 2023

Description

Adding detailed_tracing field to API Definition. If this is set to true, then each OpenTelemetry trace will propagate the context and multiple spans will be shown. Otherwise, only the main span and a span showing the request to the upstream service will be present.

Related Issue

https://tyktech.atlassian.net/browse/TT-7902?atlOrigin=eyJpIjoiMjgwMDY3YTQ5Nzk0NDAyODlkODUxOTUzMzc5NTZhZDgiLCJwIjoiaiJ9

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

  • 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 change)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

  • I ensured that the documentation is up to date
  • I explained why this PR updates go.mod in detail with reasoning why it's required
  • I would like a code coverage CI quality gate exception and have explained why

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 7, 2023

API tests result: failure 🚫
Branch used: refs/pull/5294/merge
Commit:
Triggered by: pull_request (@mativm02)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 7, 2023

API tests result: failure 🚫
Branch used: refs/pull/5294/merge
Commit: 10c54c7
Triggered by: pull_request (@mativm02)
Execution page

@mativm02 mativm02 marked this pull request as ready for review July 7, 2023 14:56
@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2023

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 7, 2023

API tests result: failure 🚫
Branch used: refs/pull/5294/merge
Commit: 45f08e1
Triggered by: pull_request (@mativm02)
Execution page

@github-actions
Copy link
Contributor

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

@github-actions
Copy link
Contributor

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

@github-actions
Copy link
Contributor

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 10, 2023

API tests result: success
Branch used: refs/pull/5294/merge
Commit: 0b50bb7
Triggered by: pull_request (@mativm02)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 10, 2023

API tests result: success
Branch used: refs/pull/5294/merge
Commit: 6027a77
Triggered by: pull_request (@mativm02)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 10, 2023

API tests result: success
Branch used: refs/pull/5294/merge
Commit: b828b57
Triggered by: pull_request (@mativm02)
Execution page

@github-actions
Copy link
Contributor

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

@github-actions
Copy link
Contributor

💥 CI tests failed 🙈

git-state

diff --git a/go.sum b/go.sum
index bc9b573..1f8e02e 100644
--- a/go.sum
+++ b/go.sum
@@ -99,8 +99,6 @@ github.com/TykTechnologies/murmur3 v0.0.0-20230310161213-aad17efd5632 h1:T5NWziF
 github.com/TykTechnologies/murmur3 v0.0.0-20230310161213-aad17efd5632/go.mod h1:UsPYgOFBpNzDXLEti7MKOwHLpVSqdzuNGkVFPspQmnQ=
 github.com/TykTechnologies/openid2go v0.1.2 h1:WXctksOahA/epTVVvbn9iNUuMXKRr0ksrF4dY9KW8o8=
 github.com/TykTechnologies/openid2go v0.1.2/go.mod h1:gYfkqeWa+lY3Xz/Z2xYtIzmYXynlgKZaBIbPCqdcdMA=
-github.com/TykTechnologies/opentelemetry v0.0.5 h1:oH21VUJJ4wW9RlyaXIIL2Av139qRKAkUahaozSkZURg=
-github.com/TykTechnologies/opentelemetry v0.0.5/go.mod h1:bltWc1OyxnIyiado+XTl85g4EbMiCUjH52W12rbW194=
 github.com/TykTechnologies/opentelemetry v0.0.6-0.20230710132847-4d8b5d5a0515 h1:gz5nvYPmSWS186uQPrLHFhXDhjxDoyUoz3dc1sqdad0=
 github.com/TykTechnologies/opentelemetry v0.0.6-0.20230710132847-4d8b5d5a0515/go.mod h1:bltWc1OyxnIyiado+XTl85g4EbMiCUjH52W12rbW194=
 github.com/TykTechnologies/storage v1.0.5 h1:lfMljPueySAW7Mpc70g1/qC5n2LKNcKgQs+Xw30apP8=

Please look at the run or in the Checks tab.

1 similar comment
@github-actions
Copy link
Contributor

💥 CI tests failed 🙈

git-state

diff --git a/go.sum b/go.sum
index bc9b573..1f8e02e 100644
--- a/go.sum
+++ b/go.sum
@@ -99,8 +99,6 @@ github.com/TykTechnologies/murmur3 v0.0.0-20230310161213-aad17efd5632 h1:T5NWziF
 github.com/TykTechnologies/murmur3 v0.0.0-20230310161213-aad17efd5632/go.mod h1:UsPYgOFBpNzDXLEti7MKOwHLpVSqdzuNGkVFPspQmnQ=
 github.com/TykTechnologies/openid2go v0.1.2 h1:WXctksOahA/epTVVvbn9iNUuMXKRr0ksrF4dY9KW8o8=
 github.com/TykTechnologies/openid2go v0.1.2/go.mod h1:gYfkqeWa+lY3Xz/Z2xYtIzmYXynlgKZaBIbPCqdcdMA=
-github.com/TykTechnologies/opentelemetry v0.0.5 h1:oH21VUJJ4wW9RlyaXIIL2Av139qRKAkUahaozSkZURg=
-github.com/TykTechnologies/opentelemetry v0.0.5/go.mod h1:bltWc1OyxnIyiado+XTl85g4EbMiCUjH52W12rbW194=
 github.com/TykTechnologies/opentelemetry v0.0.6-0.20230710132847-4d8b5d5a0515 h1:gz5nvYPmSWS186uQPrLHFhXDhjxDoyUoz3dc1sqdad0=
 github.com/TykTechnologies/opentelemetry v0.0.6-0.20230710132847-4d8b5d5a0515/go.mod h1:bltWc1OyxnIyiado+XTl85g4EbMiCUjH52W12rbW194=
 github.com/TykTechnologies/storage v1.0.5 h1:lfMljPueySAW7Mpc70g1/qC5n2LKNcKgQs+Xw30apP8=

Please look at the run or in the Checks tab.

@github-actions
Copy link
Contributor

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

@github-actions
Copy link
Contributor

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 11, 2023

API tests result: success
Branch used: refs/pull/5294/merge
Commit: 6bd64b7
Triggered by: pull_request (@mativm02)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 21, 2023

API tests result: success
Branch used: refs/pull/5294/merge
Commit: 1ee9a1f
Triggered by: pull_request (@mativm02)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 21, 2023

API tests result: success
Branch used: refs/pull/5294/merge
Commit: 3ff2a8b
Triggered by: pull_request (@mativm02)
Execution page

@@ -13,8 +13,7 @@ tasks:
sources:
- go.mod
- go.sum
- './**/*.go'

- "./**/*.go"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

change made by the linter

Comment on lines +77 to +81
set -e
task test:opentelemetry:setup
trap 'task test:opentelemetry:teardown' EXIT
task test:opentelemetry:test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm ensuring teardown is executed no matter if the test failed or not

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 24, 2023

API tests result: success
Branch used: refs/pull/5294/merge
Commit: c3284a5
Triggered by: pull_request (@mativm02)
Execution page

Copy link
Contributor

@tbuchaillot tbuchaillot left a comment

Choose a reason for hiding this comment

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

Please add a test case here with detailed spans https://github.com/TykTechnologies/tyk/blob/master/gateway/api_loader_test.go#L434

Also, please check with OAS API defs

defer span.End()
setContext(r, ctx)
} else {
span = tyktrace.SpanFromContext(r.Context())
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this function to otel internal pkg, so we don't have to import /opentelemetry/trace here

@@ -724,7 +724,10 @@ const Schema = `{
},
"is_oas": {
"type": "boolean"
}
},
"detailed_tracing": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this linted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is!

Copy link
Contributor

@sredxny sredxny left a comment

Choose a reason for hiding this comment

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

✅ lgtm! only fix what Tom suggested

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 24, 2023

API tests result: success
Branch used: refs/pull/5294/merge
Commit: 6b84656
Triggered by: pull_request (@mativm02)
Execution page

@github-actions
Copy link
Contributor

💥 CI tests failed 🙈

git-state

diff --git a/apidef/oas/schema/x-tyk-gateway.md b/apidef/oas/schema/x-tyk-gateway.md
index 9228f5e..89aac77 100644
--- a/apidef/oas/schema/x-tyk-gateway.md
+++ b/apidef/oas/schema/x-tyk-gateway.md
@@ -306,6 +306,9 @@ CustomDomain is the domain to bind this API to.
 
 Tyk classic API definition: `domain`.
 
+**Field: `detailedTracing` ([DetailedTracing](#detailedtracing))**
+DetailedTracing enables detailed tracing for this API.
+
 
 ### **ListenPath**
 
@@ -617,6 +620,12 @@ Enabled allow/disallow the usage of the domain.
 Name is the name of the domain.
 
 
+### **DetailedTracing**
+
+**Field: `enabled` (`boolean`)**
+Enabled enables detailed tracing.
+
+
 ### **Middleware**
 
 **Field: `global` ([Global](#global))**

Please look at the run or in the Checks tab.

@github-actions
Copy link
Contributor

💥 CI tests failed 🙈

git-state

diff --git a/apidef/oas/schema/x-tyk-gateway.md b/apidef/oas/schema/x-tyk-gateway.md
index 9228f5e..89aac77 100644
--- a/apidef/oas/schema/x-tyk-gateway.md
+++ b/apidef/oas/schema/x-tyk-gateway.md
@@ -306,6 +306,9 @@ CustomDomain is the domain to bind this API to.
 
 Tyk classic API definition: `domain`.
 
+**Field: `detailedTracing` ([DetailedTracing](#detailedtracing))**
+DetailedTracing enables detailed tracing for this API.
+
 
 ### **ListenPath**
 
@@ -617,6 +620,12 @@ Enabled allow/disallow the usage of the domain.
 Name is the name of the domain.
 
 
+### **DetailedTracing**
+
+**Field: `enabled` (`boolean`)**
+Enabled enables detailed tracing.
+
+
 ### **Middleware**
 
 **Field: `global` ([Global](#global))**

Please look at the run or in the Checks tab.

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 25, 2023

API tests result: success
Branch used: refs/pull/5294/merge
Commit: 7f36f49
Triggered by: pull_request (@mativm02)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 25, 2023

API tests result: success
Branch used: refs/pull/5294/merge
Commit: 6de24ee
Triggered by: pull_request (@mativm02)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 25, 2023

API tests result: success
Branch used: refs/pull/5294/merge
Commit: 6f5619b
Triggered by: pull_request (@mativm02)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 25, 2023

API tests result: success
Branch used: refs/pull/5294/merge
Commit: b17b01a
Triggered by: pull_request (@mativm02)
Execution page

@alephnull alephnull removed the request for review from a team July 26, 2023 11:28
@mativm02 mativm02 force-pushed the TT-7902 branch 2 times, most recently from b3e4a60 to 6b84656 Compare July 31, 2023 14:18
@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 31, 2023

API tests result: success
Branch used: refs/pull/5294/merge
Commit: b3e4a60
Triggered by: pull_request (@mativm02)
Execution page

@sonarcloud
Copy link

sonarcloud bot commented Jul 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

88.9% 88.9% Coverage
0.0% 0.0% Duplication

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 31, 2023

API tests result: success
Branch used: refs/pull/5294/merge
Commit: 6b84656
Triggered by: pull_request (@mativm02)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 31, 2023

API tests result: success
Branch used: refs/pull/5294/merge
Commit: 8c83546
Triggered by: pull_request (@mativm02)
Execution page

@mativm02 mativm02 merged commit 7fa737d into master Jul 31, 2023
18 of 19 checks passed
@mativm02 mativm02 deleted the TT-7902 branch July 31, 2023 17:15
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.

4 participants