Conversation
|
Created temporalio/temporal#5461 since I had to work around this a bit |
| // Grab a free port for metrics ahead-of-time so we know what port is selected | ||
| if opts.MetricsPort == 0 { | ||
| opts.MetricsPort = devserver.MustGetFreePort() | ||
| } |
There was a problem hiding this comment.
Hrmm, so looking at the dev server code, I am not seeing that metrics config is even set if no metrics port is given by the user which I wonder if that means that no metrics endpoint is even created/bound. Can you confirm what happens today if metrics port is unset (i.e. 0)?
There was a problem hiding this comment.
It is indeed created and started still - but only with LiteServer (which is the one that gets used here, right?) https://github.com/temporalio/temporal/blob/a2b17bebadc914f6445e9bd85be126f94730b25f/internal/temporalite/lite_server.go#L121
There was a problem hiding this comment.
which is the one that gets used here, right?
I don't think so. But I wonder if I introduced a regression copying over dev server code. Current CLI does default a metrics port:
Lines 153 to 155 in 7267df6
Can be convinced either way (can also get others' opinion on whether we should always start a metrics server if you want).
There was a problem hiding this comment.
I figure we might as well. I'm just not sure I see any real reason not to, and it's nice to have the metrics there if at some point after starting it you realize you wanted to see something.
There was a problem hiding this comment.
The only reason is not to bind a socket nobody uses. In current CLI they were starting a metrics server and didn't even tell anyone where to access it, so they were starting a full server unnecessarily. Now we are always starting it whether used or not.
Also I think showing the server API endpoint and the UI endpoint makes sense and is something most users will want, but a metrics endpoint is something very few users will want yet its endpoint is now printed right there with the other two important ones.
Having said all of this, I don't have a strong opinion. Worth getting anyone else's? If not, I can approve as is.
There was a problem hiding this comment.
Eh, I think the discoverability is a nice thing actually. A curious person might open it up, go "oh hey this is nice" who otherwise never would've known.
2 participants
What was changed
Log the port metrics uses on startup
Why?
Useful
Checklist
Closes [Feature Request] Log metrics endpoint's port number for visibility #309
How was this tested:
Manually
Any docs updates needed?