-
Notifications
You must be signed in to change notification settings - Fork 761
Additional metrics exported from Celery workers #3463
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
base: main
Are you sure you want to change the base?
Conversation
3755a64
to
8867346
Compare
09038ae
to
2d192ca
Compare
2d192ca
to
63b559b
Compare
2221552
to
e9e80e1
Compare
It would also be nice to have task result counters, like the following
|
99db9da
to
a417078
Compare
Agreed that it'd be nice to have. I don't want to increase the scope of this MR as it's already quite large but I can look into this in the near future. |
e3a2c0f
to
c42bbb8
Compare
...on/opentelemetry-instrumentation-celery/src/opentelemetry/instrumentation/celery/__init__.py
Outdated
Show resolved
Hide resolved
@@ -96,6 +97,12 @@ def add(x, y): | |||
_TASK_REVOKED_TERMINATED_SIGNAL_KEY = "celery.terminated.signal" | |||
_TASK_NAME_KEY = "celery.task_name" | |||
|
|||
# Metric names | |||
_TASK_COUNT_ACTIVE = "messaging.client.active_tasks" |
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.
These 3 new metrics are not in semantic conventions right? I don't think we should add them to the same namespace as the others. If adding unspecified metrics at all.
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.
Just to summarize my viewpoint
# Already in semconv
* _TASK_PROCESSING_TIME = messaging_metrics.MESSAGING_PROCESS_DURATION
# Probably should be in semconv
* _TASK_COUNT_ACTIVE = "messaging.client.active_tasks"
# Debatable as this is is more linked to Celery
* _TASK_COUNT_PREFETCHED = "messaging.client.prefetched_tasks"
* _TASK_PREFETCH_TIME = "messaging.prefetch.duration"
By different namespace, do you mean replacing messaging
by something like celery
for the last two (or three) of these?
|
||
### Fixed | ||
|
||
- `opentelemetry-instrumentation-celery` Fix a memory leak where a reference to a task identifier is kept indefinitely |
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.
Could you please extract this change in another PR so we can get this reviewed and hopefully merged before next release?
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 would say the same. The memory leak seems more feasible to review and merge.
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 opened this small PR to just fix the leak.
|
||
def create_celery_metrics(self, meter) -> None: | ||
self.metrics = { | ||
"flower.task.runtime.seconds": meter.create_histogram( |
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.
need to discuss if we can keep this and the new metric to avoid breakage.
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.
It's fine of course to keep this for backwards compatibility, albeit not sure it's worth it in this case. Metric name has nothing to do with OTel or semantic conventions, and references an unrelated project. :/
Sorry for delays - summer time and all that. I'll go through the review comments. |
0ef3755
to
d9f3fee
Compare
d9f3fee
to
1df9e8d
Compare
Description
This PR includes an enhancement regarding metrics exported from Celery workers and implements these measurements
Currently, there's only one metric being exported -
flower.task.runtime.seconds
, and it was renamed to follow semantic conventions (reported in the changelog).Changes in this PR also fix a memory leak present in current instrumentation.
Fixes #3458
Type of change
How Has This Been Tested?
Does This PR Require a Core Repo Change?
Checklist: