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

Enable Native OpenTelemetry Tracing ADR #177

Merged
merged 20 commits into from
Apr 9, 2024
Merged

Conversation

mftb
Copy link
Contributor

@mftb mftb commented Apr 2, 2024

Proposing an ADR for enabling native tracing capabilities on Konflux

@mftb mftb requested a review from a team as a code owner April 2, 2024 13:38
@mftb
Copy link
Contributor Author

mftb commented Apr 4, 2024

@gbenhaim @amisstea PTAL

@amisstea amisstea requested review from a team, ifireball and gbenhaim April 4, 2024 19:50
Copy link
Contributor

@amisstea amisstea left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Member

@ifireball ifireball left a comment

Choose a reason for hiding this comment

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

/lgtm

@mftb
Copy link
Contributor Author

mftb commented Apr 8, 2024

@ralphbean PTAL

@ralphbean ralphbean requested review from gabemontero and enarha April 8, 2024 17:54
@ralphbean
Copy link
Member

@gabemontero @enarha I'd appreciate your review here.

@gabemontero
Copy link
Contributor

@gabemontero @enarha I'd appreciate your review here.

@enarha is on PTO this week @ralphbean but I will try and look at this soon, today possibly


We are going to enable as much native tracing in Konflux as we can by quickly enabling any pre-existing tracing capabilities in the system. Any other type of tracing (e.g. zero code instrumentation or code based instrumentation) is out of scope for this ADR.

For instance, [Pipeline Service](https://github.com/redhat-appstudio/architecture/blob/main/architecture/pipeline-service.md) has to be instrumented with OTel as it provides Tekton APIs and services to Konflux. Pipeline Services includes Tekton which already natively supports [OpenTelemetry Distributed Tracing for Tasks and Pipelines](https://github.com/tektoncd/community/blob/main/teps/0124-distributed-tracing-for-tasks-and-pipelines.md), so no upstream changes are required. We just need to work to enable this native tracing (e.g. set environment variables).
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit misleading, in that the feature you note only applies to the core tekton controller.

Neither PAC, nor chains, nor results has the capability yet. At least some in Konflux consider all those "Pipeline Service".

If you want this across all of tekton, you need to duplicate tektoncd/pipeline#5746 in all those other projects.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course with open source, everyone can contribute :-) .... but if we stay in silos and expect openshift pipelines to update those other projects, updating those other projects will get prioritized against a bunch of other Konflux and non-Konflux asks, as there is not longer a dedicated team for the Konflux pipeline service mission (with it getting rolled into openshift pipelines).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I guess more accurate wording would be to state that we are enabling tracing in the core tekton controller in Konflux instead of the pipeline service then?

I'm asking because the proposed scope for this ADR is to enable as much native tracing as possible, and not add code-based instrumentation (that would be part of a follow up ADR).

Copy link
Contributor

@gabemontero gabemontero Apr 9, 2024

Choose a reason for hiding this comment

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

Then I guess more accurate wording would be to state that we are enabling tracing in the core tekton controller in Konflux instead of the pipeline service then?

yes; and then say upstream work is needed to enable this for pac, chains, and results

also, if possible, I'd like to start reducing our use of the term "pipeline service" , and just say Tekton or OpenShift Pipelines (the RH Tekton offering). At this point, all "pipeline service" really means is how Konflux configures Tekton.

I'm asking because the proposed scope for this ADR is to enable as much native tracing as possible, and not add code-based instrumentation (that would be part of a follow up ADR).

sounds good / makes sense .... that follow up notion, if not already detailed in the ADR, should be then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the ADR. PTAL.

Copy link
Contributor

Choose a reason for hiding this comment

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

realized my abbreviations may not have been expansive enough ... just in case:

  • "pac" : pipelines as code i.e. https://github.com/openshift-pipelines/pipelines-as-code/ ... this is how Konflux receives github / gitlab events and triggers the secure supply chain pipelines
  • "chains" : tekton chains i.e. https://github.com/tektoncd/chains which is leveraged by the enterprise contract team for all the attestations, signings, etc.
  • "results" : tekton results i.e. https://github.com/tektoncd/results which is how Konflux currently is offloading pipelinerun/taskrun yaml and underlying pods logs to RDS and S3 respectively so that users can access that data after those items are pruned from etcd, where we prune from etcd to both 1) avoid the scaling limits of etcd, 2) free up the PVCs currently used by the Konflux pipelines, as AWS limits on these are well beyond Konflux's needs


Native tracing will be enabled for the core Tekton controller as no upstream changes are required in order to do so, as Tekton already natively supports [OpenTelemetry Distributed Tracing for Tasks and Pipelines](https://github.com/tektoncd/community/blob/main/teps/0124-distributed-tracing-for-tasks-and-pipelines.md). We just need to work to enable this native tracing (e.g. set environment variables).

Other Tekton pieces that Konflux leverages such as [chains](https://tekton.dev/docs/chains/) and [results](https://tekton.dev/docs/results/) will have to be instrumented separately and will require upstream changes, so they are out of scope for this ADR.
Copy link
Contributor

Choose a reason for hiding this comment

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

other than listing pipelines as code here as well, I'm good with the updates; thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a pac mention.

Copy link
Contributor

@gabemontero gabemontero left a comment

Choose a reason for hiding this comment

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

just the minor add of pipelines as code for future items wrt the tekton umbrella of stuff

otherwise I'm fine with the Tekton related references

I am curious why the other Konflux components like build sevice , application service, release service, integration service, enterprise contract were not explicitly called out like Tekton was. If it is implied here, apologies, I'm missing it.

…trumentation that isn't native is out of scope for this ADR
@mftb
Copy link
Contributor Author

mftb commented Apr 9, 2024

just the minor add of pipelines as code for future items wrt the tekton umbrella of stuff

otherwise I'm fine with the Tekton related references

I am curious why the other Konflux components like build sevice , application service, release service, integration service, enterprise contract were not explicitly called out like Tekton was. If it is implied here, apologies, I'm missing it.

Explicitly mentioned a few Konflux services.

Copy link
Contributor

@gabemontero gabemontero left a comment

Choose a reason for hiding this comment

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

/lgtm

@gbenhaim gbenhaim merged commit 15460e2 into konflux-ci:main Apr 9, 2024
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.

7 participants