chore(rq): integrated core api to rq integration #10607

jessicagamio · 2024-09-10T17:26:02Z

Implementing the Core API into the existing rq integration.

Checklist

PR author has checked that all the criteria below are met
The PR description includes an overview of the change
The PR description articulates the motivation for the change
The change includes tests OR the PR description describes a testing strategy
The PR description notes risks associated with the change, if any
Newly-added code is easy to change
The change follows the library release note guidelines
The change includes or references documentation updates if necessary
Backport labels are set (if applicable)

Reviewer Checklist

Reviewer has checked that all the criteria below are met
Title is accurate
All changes are related to the pull request's stated goal
Avoids breaking API changes
Testing strategy adequately addresses listed risks
Newly-added code is easy to change
Release note makes sense to a user of the library
If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
Backport labels are set in a manner that is consistent with the release branch maintenance policy

github-actions · 2024-09-10T17:26:39Z

CODEOWNERS have been resolved as:

ddtrace/_trace/trace_handlers.py                                        @DataDog/apm-sdk-api-python
ddtrace/contrib/rq/__init__.py                                          @DataDog/apm-core-python @DataDog/apm-idm-python

datadog-dd-trace-py-rkomorn · 2024-09-10T17:44:38Z

Datadog Report

Branch report: jgamio_integrate_with_coreapi
Commit report: 50e34a6
Test service: dd-trace-py

✅ 0 Failed, 592 Passed, 694 Skipped, 39m 15.39s Total duration (34m 54.2s time saved)

pr-commenter · 2024-09-10T21:55:57Z

Benchmarks

Benchmark execution time: 2024-09-23 19:06:06

Comparing candidate commit 50e34a6 in PR branch jgamio_integrate_with_coreapi with baseline commit 7a2e802 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 353 metrics, 47 unstable metrics.

emmettbutler

Aside from the one note I left, this looks great. Nice work so far. Please also add a brief description to the pull request.

ddtrace/contrib/rq/__init__.py

…ue_fetch_job spans.

mabdinur

Let some nits but overall this looks really good. Great job!

ddtrace/_trace/trace_handlers.py

ddtrace/contrib/rq/__init__.py

ddtrace/_trace/trace_handlers.py

mabdinur

Tests are passing: https://gitlab.ddbuild.io/DataDog/apm-reliability/dd-trace-py/-/jobs/642791801.

This change looks good to me. Thanks for sticking with it

emmettbutler

This looks great. Seems like you've got your head around how the Core API works. There are a few bits that should be changed, but the main body of this change is ready to go.

emmettbutler · 2024-09-19T15:16:27Z

ddtrace/_trace/trace_handlers.py

+    config = ctx.get_item("integration_config")
+    distributed_tracing_enabled = config.distributed_tracing_enabled


Since this code is written with the (acceptable) assumption that integration_config will always exist, making that assumption more explicit would clarify the code. Example:

Suggested change

config = ctx.get_item("integration_config")

distributed_tracing_enabled = config.distributed_tracing_enabled

distributed_tracing_enabled = ctx["integration_config"].distributed_tracing_enabled

This code throws a KeyError if that key isn't present, as opposed to the less-clear NoneType has no attribute... error that the current version would throw.

emmettbutler · 2024-09-19T15:17:28Z

ddtrace/_trace/trace_handlers.py

+        span.error = 1
+    if span_tags:
+        for k in span_tags.keys():
+            span.set_tag_str(k, span_tags[k])


This code fails if span is None

emmettbutler · 2024-09-19T16:46:23Z

ddtrace/_trace/trace_handlers.py

+
+def _on_end_of_traced_method_in_fork(ctx):
+    """Force flush to agent since the process `os.exit()`s
+    immediately after this method returnsf


Suggested change

immediately after this method returnsf

immediately after this method returns

emmettbutler · 2024-09-19T16:52:39Z

ddtrace/contrib/rq/__init__.py

-        span.set_tag_str("job.id", job_id)
+        call_key="traced_queue_fetch_job",
+        tags={COMPONENT: config.rq.integration_name, JOB_ID: job_id},
+    ) as ctx, ctx[ctx["call_key"]] as _:


Suggested change

) as ctx, ctx[ctx["call_key"]] as _:

) as ctx, ctx[ctx["call_key"]]:

This does the same thing

emmettbutler · 2024-09-19T16:52:54Z

ddtrace/contrib/rq/__init__.py

+            distributed_headers_config=config.rq_worker,
+            distributed_headers=job.meta,
+            tags={COMPONENT: config.rq.integration_name, SPAN_KIND: SpanKind.CONSUMER, JOB_ID: job.get_id()},
+        ) as ctx, ctx[ctx["call_key"]] as _:


Suggested change

) as ctx, ctx[ctx["call_key"]] as _:

) as ctx, ctx[ctx["call_key"]]:

emmettbutler · 2024-09-19T16:53:59Z

ddtrace/contrib/rq/__init__.py

    finally:
        # Force flush to agent since the process `os.exit()`s
        # immediately after this method returns
-        pin.tracer.flush()
+        core.context_with_data("rq.worker.after.perform.job")


Why's this included? The dispatch on the next line seems like it does the job.

emmettbutler · 2024-09-19T16:54:16Z

ddtrace/contrib/rq/__init__.py

+        call_key="job.perform",
+        pin=pin,
+        tags={COMPONENT: config.rq.integration_name, JOB_ID: job.get_id()},
+    ) as ctx, ctx[ctx["call_key"]] as _:


Suggested change

) as ctx, ctx[ctx["call_key"]] as _:

) as ctx, ctx[ctx["call_key"]]:

emmettbutler · 2024-09-19T16:54:26Z

ddtrace/contrib/rq/__init__.py

+        call_key="job.fetch_many",
+        pin=pin,
+        tags={COMPONENT: config.rq.integration_name, JOB_ID: job_ids},
+    ) as ctx, ctx[ctx["call_key"]] as _:


Suggested change

) as ctx, ctx[ctx["call_key"]] as _:

) as ctx, ctx[ctx["call_key"]]:

…intain the automatic closing behavior.

integrated core api to rq integration

14bed80

jessicagamio added the changelog/no-changelog A changelog entry is not required for this PR. label Sep 10, 2024

jessicagamio requested review from a team as code owners September 10, 2024 17:26

jessicagamio requested review from rachelyangdog and emmettbutler September 10, 2024 17:26

Jessica Gamio added 2 commits September 10, 2024 14:19

added pin to context

5e5582e

corrected format

214bc2a

Jessica Gamio added 2 commits September 10, 2024 16:55

refactoring code

6364646

refactoring code

3903fbe

emmettbutler requested changes Sep 11, 2024

View reviewed changes

ddtrace/contrib/rq/__init__.py Outdated Show resolved Hide resolved

ddtrace/contrib/rq/__init__.py Outdated Show resolved Hide resolved

Jessica Gamio added 5 commits September 11, 2024 11:20

refactoring rq

c1bdbab

refactored code and resolved distribution trace issue

3150b11

passed lint

5b174ce

refactored rq integration for core api

b4556a2

added additional handlers for rq.worker.perform.job and rq.traced_que…

6e49d6b

…ue_fetch_job spans.

mabdinur reviewed Sep 13, 2024

View reviewed changes

Jessica Gamio added 3 commits September 16, 2024 17:10

generalizing handlers rq.worker.perform_job and rq.queue.enqueue_job

b4e2903

renamed two of the handlers

e8ae60d

added constants for some key tags

92154a7

mabdinur approved these changes Sep 17, 2024

View reviewed changes

Merge branch 'main' into jgamio_integrate_with_coreapi

dda63ed

mabdinur requested a review from emmettbutler September 18, 2024 18:57

Jessica Gamio added 2 commits September 18, 2024 12:04

added constants for span keys

b0d713e

fixed linting errors

4614bd9

emmettbutler requested changes Sep 19, 2024

View reviewed changes

removed as _ in context manager and retained ctx[ctx[call_key]] to ma…

50e34a6

…intain the automatic closing behavior.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(rq): integrated core api to rq integration #10607

chore(rq): integrated core api to rq integration #10607

jessicagamio commented Sep 10, 2024 •

edited by mabdinur

Loading

github-actions bot commented Sep 10, 2024

datadog-dd-trace-py-rkomorn bot commented Sep 10, 2024 •

edited

Loading

pr-commenter bot commented Sep 10, 2024 •

edited

Loading

emmettbutler left a comment

mabdinur left a comment •

edited

Loading

mabdinur left a comment

emmettbutler left a comment

emmettbutler Sep 19, 2024

emmettbutler Sep 19, 2024

emmettbutler Sep 19, 2024

emmettbutler Sep 19, 2024

emmettbutler Sep 19, 2024

emmettbutler Sep 19, 2024

emmettbutler Sep 19, 2024

emmettbutler Sep 19, 2024

		config = ctx.get_item("integration_config")
		distributed_tracing_enabled = config.distributed_tracing_enabled

	config = ctx.get_item("integration_config")
	distributed_tracing_enabled = config.distributed_tracing_enabled
	distributed_tracing_enabled = ctx["integration_config"].distributed_tracing_enabled

	immediately after this method returnsf
	immediately after this method returns

	) as ctx, ctx[ctx["call_key"]] as _:
	) as ctx, ctx[ctx["call_key"]]:

chore(rq): integrated core api to rq integration #10607

Are you sure you want to change the base?

chore(rq): integrated core api to rq integration #10607

Conversation

jessicagamio commented Sep 10, 2024 • edited by mabdinur Loading

Checklist

Reviewer Checklist

github-actions bot commented Sep 10, 2024

datadog-dd-trace-py-rkomorn bot commented Sep 10, 2024 • edited Loading

Datadog Report

pr-commenter bot commented Sep 10, 2024 • edited Loading

Benchmarks

emmettbutler left a comment

Choose a reason for hiding this comment

mabdinur left a comment • edited Loading

Choose a reason for hiding this comment

mabdinur left a comment

Choose a reason for hiding this comment

emmettbutler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessicagamio commented Sep 10, 2024 •

edited by mabdinur

Loading

datadog-dd-trace-py-rkomorn bot commented Sep 10, 2024 •

edited

Loading

pr-commenter bot commented Sep 10, 2024 •

edited

Loading

mabdinur left a comment •

edited

Loading