Skip to content

Move knative.dev/pkg/source.StatsReporter to eventing/pkg/metrics/source.StatsReporting - 1/n#5587

Merged
knative-prow-robot merged 3 commits into
knative:mainfrom
dprotaso:source-stats
Jul 13, 2021
Merged

Move knative.dev/pkg/source.StatsReporter to eventing/pkg/metrics/source.StatsReporting - 1/n#5587
knative-prow-robot merged 3 commits into
knative:mainfrom
dprotaso:source-stats

Conversation

@dprotaso
Copy link
Copy Markdown
Member

Part of: knative/pkg#608

tl;dr I'm moving metrickeys that are specific to a subproject out of knative.dev/pkg. Metrickeys for sources were moved to here: #5586 this moves the upstream (knative.dev/pkg/source) StatsReporter to this repo as well.

Why land this in knative/eventing?
I looked at pkg.go.dev and imports of knative.dev/pkg/source and knative.dev/pkg/metrics/metricskey is fairly minimal. All the imports I saw also pull in knative.dev/eventing thus I opt to land it here.

I'll only need to update the following repos after this PR merges

knative.dev/eventing-kafka
knative.dev/eventing-rabbitmq/pkg/adapter

Proposed Changes

  • 🧹 move knative.dev/pkg/source.StatsReporter to eventing/pkg/metrics/source.StatsReporter
  • 🧹 run update-deps.sh

Release Note

NONE

Docs

@google-cla google-cla Bot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 13, 2021
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 13, 2021
@dprotaso
Copy link
Copy Markdown
Member Author

/assign @n3wscott

@knative-metrics-robot
Copy link
Copy Markdown

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/adapter/v2/main.go 72.2% 68.9% -3.3
pkg/metrics/source/stats_reporter.go Do not exist 78.9%

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 13, 2021

Codecov Report

Merging #5587 (2a9f5e5) into main (5add485) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5587   +/-   ##
=======================================
  Coverage   82.66%   82.66%           
=======================================
  Files         198      198           
  Lines        6172     6172           
=======================================
  Hits         5102     5102           
  Misses        743      743           
  Partials      327      327           
Impacted Files Coverage Δ
pkg/adapter/v2/cloudevents.go 81.91% <ø> (ø)
pkg/adapter/v2/main.go 60.86% <ø> (ø)
pkg/adapter/v2/main_message.go 48.71% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5add485...2a9f5e5. Read the comment docs.

@knative-prow-robot knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 13, 2021
@dprotaso dprotaso changed the title Move knative.dev/pkg/source.StatsReporter to eventing/pkg/metrics/source.StatsReporting Move knative.dev/pkg/source.StatsReporter to eventing/pkg/metrics/source.StatsReporting - 1/n Jul 13, 2021
@dprotaso
Copy link
Copy Markdown
Member Author

Going to do this in three passes

  1. Define aliases to the upstream source stats reporter in eventing (this PR)
  2. Migrate the downstream implementations to use said aliases
  3. Drop the aliases in favour of the implementation living in eventing

@dprotaso
Copy link
Copy Markdown
Member Author

/retest

Copy link
Copy Markdown
Contributor

@n3wscott n3wscott 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-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2021
@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, n3wscott

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

@dprotaso
Copy link
Copy Markdown
Member Author

/retest

@dprotaso
Copy link
Copy Markdown
Member Author

broker.go:114: Internal error occurred: failed calling webhook "webhook.eventing.knative.dev": Post "https://eventing-webhook.knative-eventing-i176os6le1.svc:443/defaulting?timeout=10s": No SSH tunnels currently open. Were the targets able to accept an ssh-key for user "gke-c190e44660dd464fbfb9"?

Something's up with GKE

@knative-prow-robot knative-prow-robot merged commit 0af15fd into knative:main Jul 13, 2021
@dprotaso dprotaso deleted the source-stats branch July 13, 2021 22:10
dprotaso added a commit to dprotaso/pkg that referenced this pull request Jul 14, 2021
We're going to move the StatsReporter downstream eventually

related:
knative/eventing#5587
knative/eventing#5586
knative-prow-robot pushed a commit to knative/pkg that referenced this pull request Jul 14, 2021
* drop stack driver as a tracing backend

* drop stackdriver as a metrics backend

* update deps - dropping stackdriver

* fix linting issues

* drop further references to stack driver

* drop serving & eventing metric key constants
these have been moved to their respective repos
see: #608

* move source metrickeys to pkg/source

We're going to move the StatsReporter downstream eventually

related:
knative/eventing#5587
knative/eventing#5586

* fix linter
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. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants