From baac738726a6b615cb3f79693b0b1f117b688f9d Mon Sep 17 00:00:00 2001 From: William Conti <58711692+wconti27@users.noreply.github.com> Date: Fri, 20 Sep 2024 09:52:52 -0400 Subject: [PATCH 1/2] chore: system-tests integrations and messaging tests (#10670) Messaging / DSM tests now require AWS ## Checklist - [x] 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](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] 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](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) 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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) --- .github/workflows/system-tests.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/workflows/system-tests.yml b/.github/workflows/system-tests.yml index b8ce5cb907..e6bd299e0d 100644 --- a/.github/workflows/system-tests.yml +++ b/.github/workflows/system-tests.yml @@ -78,6 +78,10 @@ jobs: # If ever it's needed, a valid key exists in the repo, using ${{ secrets.DD_API_KEY }} DD_API_KEY: 1234567890abcdef1234567890abcdef CMAKE_BUILD_PARALLEL_LEVEL: 12 + AWS_ACCESS_KEY_ID: ${{ secrets.IDM_AWS_ACCESS_KEY_ID }} + AWS_SECRET_ACCESS_KEY: ${{ secrets.IDM_AWS_SECRET_ACCESS_KEY }} + AWS_REGION: us-east-1 + AWS_DEFAULT_REGION: us-east-1 # AWS services should use `AWS_REGION`, but some still use the older `AWS_DEFAULT_REGION` steps: - name: Checkout system tests @@ -132,6 +136,10 @@ jobs: # If ever it's needed, a valid key exists in the repo, using ${{ secrets.DD_API_KEY }} DD_API_KEY: 1234567890abcdef1234567890abcdef CMAKE_BUILD_PARALLEL_LEVEL: 12 + AWS_ACCESS_KEY_ID: ${{ secrets.IDM_AWS_ACCESS_KEY_ID }} + AWS_SECRET_ACCESS_KEY: ${{ secrets.IDM_AWS_SECRET_ACCESS_KEY }} + AWS_REGION: us-east-1 + AWS_DEFAULT_REGION: us-east-1 # AWS services should use `AWS_REGION`, but some still use the older `AWS_DEFAULT_REGION` steps: - name: Checkout system tests From 33daba967458439008e597d37802c7a00c1fed6f Mon Sep 17 00:00:00 2001 From: Zachary Groves <32471391+ZStriker19@users.noreply.github.com> Date: Fri, 20 Sep 2024 11:39:33 -0400 Subject: [PATCH 2/2] fix(kafka): cast topic to str since it can return None (#10691) Since topic can be None for a message object we should always cast it to a str or else it'll throw an error if we try to set it as a tag with set_tag_str https://docs.confluent.io/platform/current/clients/confluent-kafka-python/html/index.html#confluent_kafka.Message.topic ## 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](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] 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](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) 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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) --- ddtrace/contrib/internal/kafka/patch.py | 4 ++-- .../notes/kafka_message_topic_none_fix-2bc3fd6388075b94.yaml | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) create mode 100644 releasenotes/notes/kafka_message_topic_none_fix-2bc3fd6388075b94.yaml diff --git a/ddtrace/contrib/internal/kafka/patch.py b/ddtrace/contrib/internal/kafka/patch.py index 40f633bcf8..74a65e4c84 100644 --- a/ddtrace/contrib/internal/kafka/patch.py +++ b/ddtrace/contrib/internal/kafka/patch.py @@ -249,7 +249,7 @@ def _instrument_message(messages, pin, start_ns, instance, err): for message in messages: if message is not None and first_message is not None: - core.set_item("kafka_topic", first_message.topic()) + core.set_item("kafka_topic", str(first_message.topic())) core.dispatch("kafka.consume.start", (instance, first_message, span)) span.set_tag_str(MESSAGING_SYSTEM, kafkax.SERVICE) @@ -260,7 +260,7 @@ def _instrument_message(messages, pin, start_ns, instance, err): if first_message is not None: message_key = first_message.key() or "" message_offset = first_message.offset() or -1 - span.set_tag_str(kafkax.TOPIC, first_message.topic()) + span.set_tag_str(kafkax.TOPIC, str(first_message.topic())) # If this is a deserializing consumer, do not set the key as a tag since we # do not have the serialization function diff --git a/releasenotes/notes/kafka_message_topic_none_fix-2bc3fd6388075b94.yaml b/releasenotes/notes/kafka_message_topic_none_fix-2bc3fd6388075b94.yaml new file mode 100644 index 0000000000..05f9a9c6e5 --- /dev/null +++ b/releasenotes/notes/kafka_message_topic_none_fix-2bc3fd6388075b94.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - | + kafka: Fixed an issue where a ``TypeError`` exception would be raised if the first message's ``topic()`` returned ``None`` during consumption.