Skip to content

[python]: enable forwarder tests v2 #4924

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

betterengineering
Copy link
Member

@betterengineering betterengineering commented Jul 10, 2025

Motivation

Python has been failing the forwarder tests for some time. The injection metadata tests added by @sydney-tung also rely on this feature to be working. I've fixed the Python tracer integration with telemetry forwarding here: DataDog/dd-trace-py#13926. This change enables both tests so they can be run on the Python pipeline.

Changes

This change enables the two broken forwarder tests for python that have been fixed, which unblocks the addition of the injection metadata test.

Workflow

  1. ⚠️ Create your PR as draft ⚠️
  2. Work on you PR until the CI passes
  3. Mark it as ready for review
    • Test logic is modified? -> Get a review from RFC owner.
    • Framework is modified, or non obvious usage of it -> get a review from R&P team

🚀 Once your PR is reviewed and the CI green, you can merge it!

🛟 #apm-shared-testing 🛟

Reviewer checklist

  • If PR title starts with [<language>], double-check that only <language> is impacted by the change
  • No system-tests internal is modified. Otherwise, I have the approval from R&P team
  • A docker base image is modified?
    • the relevant build-XXX-image label is present
  • A scenario is added (or removed)?

This commit accounts for the two events we expect, one from the injector
and one from the tracer so that we can enable these tests for Python.
@betterengineering betterengineering requested a review from a team as a code owner July 10, 2025 18:11
@betterengineering betterengineering requested review from brettlangdon, pawelchcki and cbeauchesne and removed request for a team July 10, 2025 18:11
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this lgtm, and Mark and I ran these on a video call with an image with the change from dd-trace-py and both tests are passing, we also tested with the 3.10.2 artifact to confirm it is skipped, and they are skipped for older versions of Python we do not support.

Copy link
Collaborator

@robertomonteromiguel robertomonteromiguel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forced to test with the latest python tracer, and the tests are failing:

weblog_url: 
agent: 1:7.68.0-1
datadog-apm-inject: 0.39.1-dev.b0d6e40.glci945052743.ga97db900
datadog-apm-library: 3.11.0.dev82+g378df652a
docker: 
datadog-installer: 7.63.0-installer-0.12.14-rc.3
Configuration
app_type: docker_ssi
os: Ubuntu_22_amd64
arch: amd64
runtime_version: 3.8.20
Pulling ghcr.io/datadog/dd-apm-test-agent/ddapm-test-agent:v1.20.0

https://gitlab.ddbuild.io/DataDog/system-tests/-/pipelines/70267782

Let me know if you need help to solve this

@brettlangdon
Copy link
Member

@robertomonteromiguel this change depends on DataDog/dd-trace-py#13926 being merged first with the fix. So the fact that this currently fails on latest is known "blocker" 👍🏻

We haven't found a good way to merge this change first, so we can ensure it runs on the PR in dd-trace-py before merging the dd-trace-py PR. So we were going to merge the dd-trace-py PR first, and then this one (not ideal, but... do we have other options?)

@robertomonteromiguel
Copy link
Collaborator

DataDog/dd-trace-py#13926

@brettlangdon you could add the variable "SYSTEM_TESTS_REF" in the gitlab file of your python PR (configure_system_tests) and point to the latest commit hash of this system-tests PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants