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

[TEP-0124] Proposal to add distributed tracing for tekton tasks and pipelines #839

Merged
merged 5 commits into from
Oct 12, 2022

Conversation

kmjayadeep
Copy link
Contributor

Added a proposal to instrument tekton reconciler code for distributed tracing of tasks and pipelines.

Signed-off-by: Jayadeep KM kmjayadeep@gmail.com

/kind tep

@tekton-robot tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Sep 30, 2022
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 30, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: kmjayadeep / name: Jayadeep KM (1ffbaca)

@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 30, 2022

## Proposal

Initialize a tracer provider with jaeger as the backend for each reconciler. The jaeger collector URL can be passed as an argument to the controller.
Copy link
Member

Choose a reason for hiding this comment

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

Passing the URL as an argument to the controller should be enough for development environments.
Eventually, it can be an enhancement out of scope for this initial TEP, we might want to implement reading configuration from a config map, so that configuration may be altered at runtime, similar to https://pkg.go.dev/knative.dev/pkg/tracing. knative/pkg relies on OpenCensus for now, and we would like to to start using OpenTelemetry right from the beginning.

@afrittoli
Copy link
Member

/assign @pritidesai
/assign @pxp928

@tekton-robot
Copy link
Contributor

@afrittoli: GitHub didn't allow me to assign the following users: pxp928.

Note that only tektoncd members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @pritidesai
/assign @pxp928

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 3, 2022
Signed-off-by: Jayadeep KM <kmjayadeep@gmail.com>
Signed-off-by: Jayadeep KM <kmjayadeep@gmail.com>
Signed-off-by: Jayadeep KM <kmjayadeep@gmail.com>
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2022
@kmjayadeep kmjayadeep changed the title Proposal to add distributed tracing for tekton tasks and pipelines [TEP-0124] Proposal to add distributed tracing for tekton tasks and pipelines Oct 4, 2022
Signed-off-by: Jayadeep KM <kmjayadeep@gmail.com>
@afrittoli
Copy link
Member

/test pull-community-teps-lint

@tekton-robot
Copy link
Contributor

@afrittoli: No presubmit jobs available for tektoncd/community@main

In response to this:

/test pull-community-teps-lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Member

@pxp928 pxp928 left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few comments.

* Propagate traces so that reconciles of a resource owned by a parent resource belong a parent span from the parent resource
* Reconcile of different resources must belong to separate traces

## Proposal
Copy link
Member

Choose a reason for hiding this comment

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

A small architecture diagram of how jaeger integrates with tekton

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 not sure how to come up with an architecture diagram for this. There are only two relevant components, Tekton operator and Jaeger. Tekton operator just records the spans during reconcile process and flush them to jaeger on specific intervals.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. Just wanted to make sure if there was anything out of the ordinary being introduced.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @pxp928 - a diagram would be nice indeed.
We could add a diagram in the next PR when we propose this as implementable if that's ok

@dibyom
Copy link
Member

dibyom commented Oct 10, 2022

/assign

Signed-off-by: Jayadeep KM <kmjayadeep@gmail.com>
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 10, 2022
@kmjayadeep kmjayadeep requested review from pxp928 and removed request for chmouel and piyush-garg October 10, 2022 21:46
@kmjayadeep
Copy link
Contributor Author

not sure why github removed the reviewers when I was trying to request re-review. Could someone add them back please...

@afrittoli
Copy link
Member

not sure why github removed the reviewers when I was trying to request re-review. Could someone add them back please...

Assignees are still there, all seems fine :)

@abayer
Copy link
Contributor

abayer commented Oct 11, 2022

/approve

Looking forward to seeing where this goes!

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2022
Copy link
Member

@pxp928 pxp928 left a comment

Choose a reason for hiding this comment

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

LGTM
/approve

@dibyom
Copy link
Member

dibyom commented Oct 12, 2022

/approve

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer, dibyom, pxp928, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dibyom
Copy link
Member

dibyom commented Oct 12, 2022

/lgtm

(Merging since all approvals have been met)

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2022
@tekton-robot tekton-robot merged commit 5a521d8 into tektoncd:main Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: UnAssigned
Development

Successfully merging this pull request may close these issues.

8 participants