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 metrics for sources in Knative-eventing using Prometheus #4736

Closed
kthakar1990 opened this issue Jan 13, 2021 · 7 comments · Fixed by #5115
Closed

Enable metrics for sources in Knative-eventing using Prometheus #4736

kthakar1990 opened this issue Jan 13, 2021 · 7 comments · Fixed by #5115
Assignees
Labels
area/observability area/sources priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Milestone

Comments

@kthakar1990
Copy link
Contributor

kthakar1990 commented Jan 13, 2021

Expected Behavior

This is a request for adding feature for metrics-related tasks in importers (event_count - already present, event_processing_latencies - needed, retry_event_count- needed):

  • ApiServerSource
  • PingJobSource
  • KafkaSource
  • GitHubSource

Persona
Which persona is this feature for?
Event consumer, Event producer, and Operator.

Exit Criteria
Users should be able to see metrics in the grafana/prometheus UI and stackdriver. Documentation should be available in knative.dev, indicating what metrics are defined and how to add new ones, as well as how to configure the metrics infrastructure. E2E tests should be in place as well.

@lionelvillard
Copy link
Member

/assign kthakar1990

@kthakar1990 kthakar1990 changed the title Enable metrics for Importers in Knative-eventing using Prometheus Enable metrics for sources in Knative-eventing using Prometheus Jan 19, 2021
@lberk lberk added area/observability area/sources priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Jan 25, 2021
@lberk lberk added this to the v0.21.0 milestone Jan 25, 2021
@slinkydeveloper
Copy link
Contributor

cc @skonto for feedback

@skonto
Copy link
Contributor

skonto commented Jan 28, 2021

@kthakar1990 feel free to add the required metrics via a PR. I will also share asap a doc to collaborate on defining what metrics Serving/Eventing requires to be exposed and what is the current status so we can work on this.
/cc @aliok @bharattkukreja @evankanderson

@kthakar1990
Copy link
Contributor Author

@skonto I was told by @vaikas and others that you're working on a PR and I can work with you to get my changes added to that PR.. So, for clarification when you said feel free to add the required metrics via PR.. is there an existing code or a PR that you have submitted where I am supposed to add my changes or are you asking me to go on about this requirement independently. I would not mind either way but just wanted to understand more.. Also, since newer metrics addition would be via opentelemetry work if there is a code out there for it or WIP.. can you point me to that?

@skonto
Copy link
Contributor

skonto commented Jan 29, 2021

@kthakar1990 I replied on slack as well. You could add your metrics in the current implementation on main branch, my PoC is irrelevant to the metrics you want to add. I guess there is a misunderstanding, what I am working on is evaluating of moving to open telemetry go lib at some point. At some point this migration may affect existing metrics but its too early.
I am not working on adding the metrics you need in the description. If you want to add event_processing_latencies, event_error_count then you need to expand the reporter here:

. This reporter is started in each source adapter's main here:
func NewStatsReporter() (StatsReporter, error) {
. Same story afaik with external sources like github, https://github.com/knative-sandbox/eventing-github/blob/master/cmd/mt_receive_adapter/main.go they use the same main func.
There is also work in process for deciding upon a metrics api/contract which in the future will affect the current one but this is not done so from my side feel free to issue a PR with what you need. /cc @evankanderson

@grantr
Copy link
Contributor

grantr commented Mar 8, 2021

This issue is still in v0.21.0. Should we move it to a future release milestone or to the backlog?

@kthakar1990
Copy link
Contributor Author

@grantr I have raised a PR that addresses this issue.. once that gets merged..we can close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/observability area/sources priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants