Pass through global telemetry tags #303
Conversation
|
|
|
|
temporalio/bridge/sdk-core
Outdated
There was a problem hiding this comment.
Unfortunately this is going to incur a larger cost. Core changed shutdown mechanisms and a couple of other things. I have a commit at main...cretz:temporal-sdk-python:core-update that will update everything to latest core, but we need to get temporalio/sdk-core#519 done first.
Once that core issue is fixed and I can then update core, we can revisit this.
There was a problem hiding this comment.
Yeah, I figured the build fix might take some effort.
Let me know with a ping when we can revisit.
There was a problem hiding this comment.
@cretz looks to be merged. Rebased on your main
There was a problem hiding this comment.
Awesome, running CI then will merge if/when it passes.
| @@ -175,6 +175,8 @@ class TelemetryConfig: | |||
| metrics: Optional[Union[OpenTelemetryConfig, PrometheusConfig]] = None | |||
| """Metrics configuration.""" | |||
|
|
|||
| global_tags: Mapping[str, str] = field(default_factory=dict) | |||
There was a problem hiding this comment.
Are these tags applied to metrics and tracing? Does that include Prometheus metrics and OTel tracing?
There was a problem hiding this comment.
Yes to all of the above.
I did not test prometheus myself (as we don't use it) but I do pass these through to the initializer
There was a problem hiding this comment.
Oh, just realized. Can we at least add one line of docs here?
ytaben
force-pushed
the
ytaben/metric-tags
branch
from
3a0dc4d to
52045ec
Compare
ytaben
force-pushed
the
ytaben/metric-tags
branch
from
8f5bc60 to
eb556cc
Compare
|
@cretz Looks like CI is now passing (after some retries it seems) |
|
Yup, merging. Thanks! |
What was changed
This PR adds python representation of
global_tagsand passes them through to rust versionWhy?
See temporalio/sdk-core#518 for the base PR in the core SDK. This PR just adds means to set the new parameter from Python
The result should allow us to identify resources reporting metrics (i.e. service name, host ID, environment, etc)
Checklist
Closes (closed the issue in sdk-core. Doesn't seem to be one here)
How was this tested:
Tested in the original PR
Any docs updates needed?
Unlikely (unless we document telemetry fields somewhere)
Note: I bumped the
sdk-coresubmodule to currentmaster. The project does not build with that version (and did not seem to build even before I touched it). I did my tests with my work applied on top of the commit referenced in the current submodule.My current understanding is that this is not caused by me and needs to be addressed by the maintainers at some point.