Skip to content
This repository has been archived by the owner on Dec 21, 2023. It is now read-only.

feat: Add new keptn version with context propagation in place #336

Conversation

joaopgrassi
Copy link
Contributor

This PR

Introduces a new version of keptn lib with Go context propagation in the methods/functions.

Related Issues

Part of keptn/enhancement-proposals#30 (comment)

Notes

The need for this comes with our intention of adding OpenTelemetry instrumentation to Keptn. The way context propagation works in OpenTelemetry for Go is via the context.Context object. It is used to pass along cross-cutting concerns, such as cancellation and now, the tracecontext information.

Since changing the existing methods will incur a breaking API change, a new version of the package is needed.

Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com>
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
18.0% 18.0% Duplication

ctx = cloudevents.ContextWithTarget(ctx, httpSender.EventsEndpoint)
}

// TODO: Should we also add cloudevents.WithEncodingStructured(ctx) here to avoid duplication?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bacherfl @warber what you think about this? Should we always set cloudevents.WithEncodingStructured(ctx) in here, as it was done before in SendEvent? This will make transitioning to this version easier.

I get that by doing so we lose flexibility in case we want to use binary encoding later. But is that even the case? I tried checking if such encoding config is already present, but as my comment says the key added to ctx is not exported in cloudevents.

@joaopgrassi joaopgrassi changed the title Add new keptn version with context propagation in place feat: Add new keptn version with context propagation in place Sep 23, 2021
@joaopgrassi
Copy link
Contributor Author

joaopgrassi commented Sep 23, 2021

We have to also think what to do with the code in pkg/api/utils. There's several methods there that performs http requests such as creating a project.

In the POC instrumentation PR , all the HTTP clients used inside utils, are instrumented with OpenTelemetry. This means that all requests made from them will generate a span automatically.

The issue though is: if those requests are part of a larger transaction, then the spans produced by them will be not part of the trace.

For ex: If the DeleteProject is called due to another operation elsewhere, the call to it here will generate a span "outside" of the trace that originated it.

It might be the case that we are not interested in the spans generated by these files here, but that's something I really cannot tell. If that is the case, then we don't even need to add auto-instrumentation to the HTTP clients used inside utils.

@joaopgrassi
Copy link
Contributor Author

joaopgrassi commented Sep 24, 2021

Going to favor this in favor of adding the context inside the Keptn struct, as one of approaches described in the articles: https://go.dev/blog/context-and-structs, https://go.dev/blog/module-compatibility and this GH issue: golang/go#22602. A follow up PR for this version is coming up next.

@joaopgrassi joaopgrassi deleted the feat/2422/vnext_ctx_propagation branch September 24, 2021 11:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant