Skip to content

Drop OpenCensus use in various test packages #3202

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.

Sign up for GitHub

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

Jump to bottom
Merged
knative-prow merged 4 commits into from
Jul 4, 2025
Merged

Drop OpenCensus use in various test packages #3202

knative-prow merged 4 commits into from
Jul 4, 2025

Conversation

@dprotaso
Copy link
Member

@dprotaso dprotaso commented Jul 2, 2025

  • drop opencensus from test/logging and test/spoof
  • run ./hack/update-dep.sh

Merge #3189 first

dprotaso added 2 commits July 2, 2025 11:32
@dprotaso
drop opencensus from test/logging and test/spoof
0182ea6 
I don't believe we are exporting traces in any of our OSS tests

For test/logging we're just returning a noop span for now
@dprotaso
run ./hack/update-dep.sh
f1d4cc9
@knative-prow knative-prow bot requested review from Leo6Leo and mgencur July 2, 2025 15:33
@knative-prow knative-prow bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 2, 2025
@dprotaso
Copy link
Member Author

dprotaso commented Jul 2, 2025

/hold

@knative-prow knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 2, 2025
@codecov
Copy link

codecov bot commented Jul 2, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@13b2dc9). Learn more about missing BASE report.
Report is 4 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #3202   +/-   ##
=======================================
  Coverage        ?   76.07%           
=======================================
  Files           ?      205           
  Lines           ?    11751           
  Branches        ?        0           
=======================================
  Hits            ?     8940           
  Misses          ?     2539           
  Partials        ?      272

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dprotaso
Copy link
Member Author

dprotaso commented Jul 2, 2025

/assign @evankanderson @Cali0707 @skonto

evankanderson
evankanderson reviewed Jul 2, 2025
View reviewed changes
Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

/approve

One small question, but I'm happy to LGTM when it's time.

test/logging/logging.go Outdated
func GetEmitableSpan(ctx context.Context, metricName string) *trace.Span {
_, span := trace.StartSpan(ctx, emitableSpanNamePrefix+metricName)
func GetEmitableSpan(ctx context.Context, metricName string) trace.Span {
_, span := noopTracer.Start(ctx, metricName)
Copy link
Member

@evankanderson evankanderson Jul 2, 2025

Choose a reason for hiding this comment

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

Why is this always noopTracer? Is that because this is in the testing package?

Maybe put a comment on line 36 about the why of noopTracer.

Copy link
Member Author

@dprotaso dprotaso Jul 2, 2025

Choose a reason for hiding this comment

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

Why is this always noopTracer? Is that because this is in the testing package?

We had tracing in our tests WAAAY back. I don't think we use it for anything anymore so I was thinking of removing this but there's quite a few places this method is used downstream and don't think it's worth breaking right now.

So I figured I'd make this a noop.

I can create a follow up issue if you prefer

Copy link
Member Author

@dprotaso dprotaso Jul 4, 2025

Choose a reason for hiding this comment

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

I'll just leave a regular tracer for now.

We should figure out what utility this has in a follow up.

Copy link
Member

@Cali0707 Cali0707 Jul 4, 2025

Choose a reason for hiding this comment

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

+1 on figuring what the utility of this is as a follow up

@evankanderson
Copy link
Member

evankanderson commented Jul 2, 2025

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2025
@knative-prow knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 4, 2025
@dprotaso
Copy link
Member Author

dprotaso commented Jul 4, 2025

/hold cancel

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 4, 2025
Cali0707
Cali0707 approved these changes Jul 4, 2025
View reviewed changes
Copy link
Member

@Cali0707 Cali0707 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

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 4, 2025
@knative-prow
Copy link

knative-prow bot commented Jul 4, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, dprotaso, evankanderson

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

The pull request process is described here

Details 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

@knative-prow knative-prow bot merged commit 7a5377f into knative:main Jul 4, 2025
36 of 38 checks passed
@dprotaso dprotaso deleted the test-drop-opencensus branch July 4, 2025 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@evankanderson evankanderson evankanderson left review comments

@Cali0707 Cali0707 Cali0707 approved these changes

@Leo6Leo Leo6Leo Awaiting requested review from Leo6Leo

@mgencur mgencur Awaiting requested review from mgencur

Assignees

@skonto skonto

@evankanderson evankanderson

@Cali0707 Cali0707

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dprotaso @evankanderson @Cali0707 @skonto