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
refactor: add metrics endpoint (DEV-1555) #2331
Conversation
✅ Linked to Story DEV-1555 · Add prometheus metrics endpoint |
Codecov ReportBase: 86.68% // Head: 85.85% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2331 +/- ##
==========================================
- Coverage 86.68% 85.85% -0.83%
==========================================
Files 250 238 -12
Lines 28252 27924 -328
==========================================
- Hits 24490 23975 -515
- Misses 3762 3949 +187
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
LGTM
|
||
override def run = for { | ||
f1 <- PrometheusServer.make.forkDaemon | ||
f2 <- (AppServer.live *> ZIO.never).forkDaemon |
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 it possible to leave AppServer.live.launch
instead of AppServer.live *> ZIO.never
?
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.
.launch
is a method on ZLayer
. It does exactly the same.
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.
Looks good code-wise, I would like to have seen some documentation on it
|
||
object PrometheusServer { | ||
|
||
private val nThreads = 5 |
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.
why is this not coming from the config?
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'm not sure what it does exactly. I still need to figure that out first. Maybe we don't need it.
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.
Looks good. Feel free to apply my suggestion. Shouldn't PrometheusServer
has some unit tests written?
f1 <- PrometheusServer.make.forkDaemon | ||
f2 <- (AppServer.live *> ZIO.never).forkDaemon | ||
_ <- f1.join | ||
_ <- f2.join |
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.
f1 <- PrometheusServer.make.forkDaemon | |
f2 <- (AppServer.live *> ZIO.never).forkDaemon | |
_ <- f1.join | |
_ <- f2.join | |
_ <- PrometheusServer.make.forkDaemon.join | |
_ <- (AppServer.live *> ZIO.never).forkDaemon.join |
How about join in the same lines?
As discussed. I would prefer to have all zio http server definitions in the core package side by side. I do think it will make sense to split the application and instrumentation http endpoint to be on separate port, hence the new Server should be named Please also move the health route to the instrumentation server. |
…ch-swiss/dsp-api into wip/DEV-1555-add-prometheus-endpoint
Docs added. |
done. |
Pull Request Checklist
Task Description/Number
Issue Number: DEV-1555
Basic Requirements
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
Does this PR change client-test-data?