-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
enhance metric support for product catalog service #231
Conversation
@cartersocha, please review. current implementation is not a push style, we really need it? it will add a new component,a Prometheus Pushgateway, I think it more costly, a static config for promethues is ok for me. |
src/productcatalogservice/server.go
Outdated
@@ -76,6 +80,30 @@ func init() { | |||
if err != nil { | |||
log.Warnf("could not parse product catalog") | |||
} | |||
|
|||
//init runtime metrics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this using the prometheus client for Go? Should we not use the OTel library for Go instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, should use otel library.
I think push gateway is not needed, we should use otel library to export metrics to otel-collector, the prometheus already scraped metrics from collector. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be using the OTel Go Metrics library rather than Prometheus.
ok, I will use the OTel library to do it |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Any updates on this? |
I want wait this issue finish open-telemetry/opentelemetry-go-contrib#2624, its a new version of instrumentation |
Does that issue have anyone actively working on it? It doesn't seem like it does... could we just use whatever the current version of go otel metrics instrumentation is, then when the new version comes out, update to that? |
# Conflicts: # src/productcatalogservice/main.go
@@ -40,4 +40,5 @@ COPY ./src/productcatalogservice/products.json ./ | |||
COPY --from=builder /go/bin/productcatalogservice/ ./ | |||
|
|||
EXPOSE ${PRODUCT_SERVICE_PORT} | |||
EXPOSE 2112 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add this port to the .env
file, just to have more control over everything?
@@ -16,53 +16,129 @@ package main | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to merge main into your branch.
This PR is undoing a couple of changes that we have merged already.
Closing as the product catalog service was overhauled and will need to be updated. |
Fixes #66
Changes
supprot runtime metrics for productcatalogservice.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes