refactor: migrate from OpenCensus to OpenTelemetry#2567
Merged
zakisk merged 1 commit intotektoncd:mainfrom Mar 25, 2026
Merged
Conversation
🔍 PR Lint Feedback
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request modernizes the project's metric collection and reporting infrastructure by transitioning from OpenCensus to OpenTelemetry. This ensures that pipeline run metrics are captured and exposed using a more current and widely adopted telemetry framework, improving future compatibility and integration possibilities. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly migrates the metrics from OpenCensus to OpenTelemetry. The dependencies in go.mod and go.sum are updated accordingly, and the implementation in pkg/pipelinerunmetrics/metrics.go is changed to use the OpenTelemetry API. The tests are also updated to use the OpenTelemetry SDK's testing utilities, which is great.
I have identified one high-severity issue with the implementation of the pipelines_as_code_running_pipelineruns_count metric, which I've detailed in a specific comment. It's a gauge-type metric but is implemented as a counter, which will lead to incorrect values.
7dfeb9b to
93ded13
Compare
93ded13 to
b7bff54
Compare
Member
|
fixed the conflicts on your PR directly, from a quick look on it i think you are missing the update to watcher? |
chmouel
reviewed
Mar 19, 2026
pkg/pipelinerunmetrics/metrics.go
Outdated
| return err | ||
| attribs := []attribute.KeyValue{ | ||
| attribute.String("provider", provider), | ||
| attribute.String("event", event), |
Member
There was a problem hiding this comment.
it used before this called event-type as per documentation, not sure if it will breaks anything or that's on purpose but maybe we want to keep it as is
https://pipelinesascode.com/docs/operations/metrics/
chmouel
reviewed
Mar 19, 2026
| } | ||
| R.provider = provider | ||
|
|
||
| eventType, errRegistering := tag.NewKey("event-type") |
chmouel
reviewed
Mar 19, 2026
| fieldRef: | ||
| fieldPath: metadata.namespace | ||
| - name: K_METRICS_CONFIG | ||
| value: '{"Domain":"pipelinesascode.tekton.dev/controller","Component":"pac_controller","PrometheusPort":9090,"ConfigMap":{"name":"pipelines-as-code-config-observability"}}' |
Member
There was a problem hiding this comment.
we dont read anymore form the configmap ? that change things compared as before. (and i think doc would need to be updated upstream/downstream)
f38f89f to
534a6e7
Compare
Member
Author
|
@chmouel there was an issue I fixed in recent changes, will merge it after CI green. feel free to comment if anything. |
534a6e7 to
e2606fd
Compare
chmouel
approved these changes
Mar 25, 2026
Replace OpenCensus stats, views, and tags with OpenTelemetry meter, counters, and attributes for all pipeline run metrics. Update method signatures to accept context.Context and rewrite tests to use the OTel SDK ManualReader. Remove the now-unnecessary metricstest helper package and all OpenCensus vendor dependencies. Signed-off-by: Zaki Shaikh <zashaikh@redhat.com>
e2606fd to
164f87b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace OpenCensus stats, views, and tags with OpenTelemetry meter, counters, and attributes for all pipeline run metrics. Update method signatures to accept context.Context and rewrite tests to use the OTel SDK ManualReader. Remove the now-unnecessary metricstest helper package and all OpenCensus vendor dependencies.
📝 Description of the Change
👨🏻 Linked Jira
🔗 Linked GitHub Issue
Fixes #
🧪 Testing Strategy
🤖 AI Assistance
If you have used AI assistance, please provide the following details:
Which LLM was used?
Extent of AI Assistance:
Important
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Gemini gemini@google.com
Co-authored-by: ChatGPT noreply@chatgpt.com
Co-authored-by: Claude noreply@anthropic.com
Co-authored-by: Cursor noreply@cursor.com
Co-authored-by: Copilot Copilot@users.noreply.github.com
**💡You can use the script
./hack/add-llm-coauthor.shto automatically addthese co-author trailers to your commits.
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.