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

add prometheus service into docker compose and add a basic metric to frontend #160

Merged

Conversation

fatsheep9146
Copy link
Contributor

@fatsheep9146 fatsheep9146 commented Jun 20, 2022

add prometheus service into docker compose and add a basic metric to frontend

Signed-off-by: Ziqi Zhao [email protected]

Fixes #70

Changes

  1. add prometheus service into docker compose
  2. add a basic metric http.server.request_count to frontend

I attempted to add metric support to webstore demo in my own mind. The implementation may be too simple. I only add one metric for frontend service and add a prometheus service to scrape metric from otel-collector. I really hope the comminuty can give more advises, for examples,

  • add more metrics
  • add more metric visualization tool, such as grafana

@fatsheep9146 fatsheep9146 requested a review from a team June 20, 2022 04:59
@fatsheep9146
Copy link
Contributor Author

fatsheep9146 commented Jun 20, 2022

add some screen shot about the metric scraped by prometheus.

you can view prometheus ui in http://127.0.0.1:9090

image

image

@fatsheep9146
Copy link
Contributor Author

@cartersocha @puckpuck @julianocosta89 Could you help review this ?

@julianocosta89
Copy link
Member

@fatsheep9146 could you take a look at the yaml lint and sanity check issues?
Regarding the PR itself it looks good, but I do not have much experience with metrics, maybe some of the other approvers have more experience, so I'd rather wait a bit and see if we get any insights from them.

@cartersocha
Copy link
Contributor

FYI Austin & I are at OTel community day on Monday & won’t be able to take a look until later this week

thanks for making progress on this tho @fatsheep9146. It’s an area that needs a lot more attention

@reyang
Copy link
Member

reyang commented Jun 20, 2022

Regarding the overall direction of metrics: #70 (comment) mentioned that "services should use push metrics where possible" which sounds like OTLP exporter scenario?

Here is my understanding:

                       +-------------------------+
SDK + OTLP Exporter ---> OpenTelemetry Collector <----- Prometheus Scraper ----> TSDB
                       +------------+------------+
                                    |
                                    +---------> Vendor Ingestion

@fatsheep9146
Copy link
Contributor Author

exporter

Regarding the overall direction of metrics: #70 (comment) mentioned that "services should use push metrics where possible" which sounds like OTLP exporter scenario?

Here is my understanding:

                       +-------------------------+
SDK + OTLP Exporter ---> OpenTelemetry Collector <----- Prometheus Scraper ----> TSDB
                       +------------+------------+
                                    |
                                    +---------> Vendor Ingestion

Yes, this is same as my understanding.

src/frontend/main.go Outdated Show resolved Hide resolved
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM with some non-blocking suggestions.

Signed-off-by: Ziqi Zhao <[email protected]>
Signed-off-by: Ziqi Zhao <[email protected]>
Signed-off-by: Ziqi Zhao <[email protected]>
src/otelcollector/otelcol-config.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
Copy link
Member

@wph95 wph95 left a comment

Choose a reason for hiding this comment

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

LGTM

@cartersocha
Copy link
Contributor

@austinlparker Are we okay using Prometheus as our metric consumption tool?

I know we want to produce OTel metrics from instrumentation. I’m not a go expert but it looks like this update uses OTLP metric submission which should meet that requirement?

Then it’s just if we prefer Prometheus over other tools

@austinlparker
Copy link
Member

I think prometheus is probably ok -- realistically, what are our options here in terms of a sane default? I think the req behind the req is that we need to demo both pull and push metrics; the actual place they get sent isn't really that important from our perspective. An end-user should be able to swap out the prom exporter with any other OTLP metrics endpoint and it'd just work, right?

@reyang
Copy link
Member

reyang commented Jun 21, 2022

I think prometheus is probably ok -- realistically, what are our options here in terms of a sane default? I think the req behind the req is that we need to demo both pull and push metrics; the actual place they get sent isn't really that important from our perspective. An end-user should be able to swap out the prom exporter with any other OTLP metrics endpoint and it'd just work, right?

I would suggest Prometheus as the TSDB (Timeseries Database) and Grafana as the UI. Here is one example in OpenTelemetry C++ and I found Grafana UI to be much more usable than Prometheus UI https://github.com/open-telemetry/opentelemetry-cpp/blob/a847d0ce42209e458f8f98a5751626a33adfad95/examples/prometheus/README.md.

Signed-off-by: Ziqi Zhao <[email protected]>
Copy link
Contributor

@puckpuck puckpuck left a comment

Choose a reason for hiding this comment

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

1 small nit pic but LGTM with the histogram metric.

src/otelcollector/otelcol-config.yml Outdated Show resolved Hide resolved
Signed-off-by: Ziqi Zhao <[email protected]>
@fatsheep9146
Copy link
Contributor Author

I think all conversations are resolved, and ready to be merged. :) @cartersocha

@cartersocha
Copy link
Contributor

Ok merging now. Let’s get a basic grafana dashboard running soon to consume this. I think this is using Prometheus’s front end still but no need to block.

@cartersocha
Copy link
Contributor

@fatsheep9146 could you separately update the change log to reflect metrics being added. I think this qualifies as significant enough

@cartersocha cartersocha merged commit c32595f into open-telemetry:main Jun 22, 2022
@fatsheep9146
Copy link
Contributor Author

@fatsheep9146 could you separately update the change log to reflect metrics being added. I think this qualifies as significant enough

Absolutely, except changelog , I think I should also update the ReadMe of the Webstore and the frontend service?

@fatsheep9146
Copy link
Contributor Author

Ok merging now. Let’s get a basic grafana dashboard running soon to consume this. I think this is using Prometheus’s front end still but no need to block.

I will work on this right now

@cartersocha
Copy link
Contributor

@fatsheep9146 could you separately update the change log to reflect metrics being added. I think this qualifies as significant enough

Absolutely, except changelog , I think I should also update the ReadMe of the Webstore and the frontend service?

Yeah let’s flesh out the documentation. Maybe we set up dedicated signal docs that holds all the links to various implementations & the consumption / storage details too?

In each service’s readme we can keep the generic feature table then some code highlights

@cartersocha
Copy link
Contributor

No need to start this structure now but something for us to consider in the next couple weeks as we clean stuff up

GaryPWhite pushed a commit to wayfair-contribs/opentelemetry-demo that referenced this pull request Jun 30, 2022
…frontend (open-telemetry#160)

* add prometheus service into docker compose and add a basic metric to frontend service

Signed-off-by: Ziqi Zhao <[email protected]>

* fix lint errors

Signed-off-by: Ziqi Zhao <[email protected]>

* fix for comments

Signed-off-by: Ziqi Zhao <[email protected]>

* fix for yamllint error

Signed-off-by: Ziqi Zhao <[email protected]>

* add http request duration metric

Signed-off-by: Ziqi Zhao <[email protected]>

* fix for comments

Signed-off-by: Ziqi Zhao <[email protected]>

* fix for comments

Signed-off-by: Ziqi Zhao <[email protected]>

Co-authored-by: Carter Socha <[email protected]>
@aengusrooneygrafana
Copy link

@fatsheep9146 the dashboard provider is missing. Will I raise an issue or a PR to fix this? You need to add a block for your dashboard provider, in addition to providing your dashboard.json.

jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
…frontend (open-telemetry#160)

* add prometheus service into docker compose and add a basic metric to frontend service

Signed-off-by: Ziqi Zhao <[email protected]>

* fix lint errors

Signed-off-by: Ziqi Zhao <[email protected]>

* fix for comments

Signed-off-by: Ziqi Zhao <[email protected]>

* fix for yamllint error

Signed-off-by: Ziqi Zhao <[email protected]>

* add http request duration metric

Signed-off-by: Ziqi Zhao <[email protected]>

* fix for comments

Signed-off-by: Ziqi Zhao <[email protected]>

* fix for comments

Signed-off-by: Ziqi Zhao <[email protected]>

Co-authored-by: Carter Socha <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review and enhance metric support for frontend (Go)
9 participants