Skip to content

parametric: Ensure start span endpoints align with the spec#3300

Merged
mabdinur merged 10 commits into
mainfrom
munir/simplify-start-span
Nov 15, 2024
Merged

parametric: Ensure start span endpoints align with the spec#3300
mabdinur merged 10 commits into
mainfrom
munir/simplify-start-span

Conversation

@mabdinur
Copy link
Copy Markdown
Contributor

@mabdinur mabdinur commented Oct 23, 2024

Motivation

  • Ensure the arguments supplied to the start span parametric endpoint are equivalent to the arguments expected by the Tracer.start_span (in most languages).

Changes

  • Adds extract_header endpoint to all parametric apps defined in the the system-tests repo (so not c++)
  • Adds a test case for the new extract_header endpoints in parametric_endpoint_parity feature.
  • Removes HttpHeader extraction from the start span endpoint
  • Removes links and origin arguments from start span.
    • dd_origin should be set using the new extract_headers endpoint via http headers (and not by start_span).
    • links should be added using the add_link endpoint. Most tracers do not support creating spans with links
  • Updates all distributed tracing tests to use extract_header API (instead of using start_span + http_headers argument)
  • Updates all tests that initialize a span with origin and links.

Workflow

  1. ⚠️ Create your PR as draft ⚠️
  2. Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
  3. Mark it as ready for review
    • Test logic is modified? -> Get a review from RFC owner. We're working on refining the codeowners file quickly.
    • Framework is modified, or non obvious usage of it -> get a review from R&P team

🚀 Once your PR is reviewed, 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
  • CI is green, or failing jobs are not related to this change (and you are 100% sure about this statement)
  • A docker base image is modified?
    • the relevant build-XXX-image label is present
  • A scenario is added (or removed)?

@mabdinur mabdinur force-pushed the munir/simplify-start-span branch 2 times, most recently from b7853fe to 3b111cd Compare October 23, 2024 23:30
@mabdinur mabdinur force-pushed the munir/simplify-start-span branch from ba968da to 1e7ebde Compare November 12, 2024 02:39
@mabdinur mabdinur changed the title parametric: simplify start span parametric: Ensure all start span endpoints align with the spec Nov 13, 2024
Comment thread utils/build/docker/golang/parametric/datadog.go
@mabdinur mabdinur force-pushed the munir/simplify-start-span branch from 8fc8570 to 03b26fc Compare November 13, 2024 19:13
@mabdinur mabdinur force-pushed the munir/simplify-start-span branch from 03b26fc to 7a8b07e Compare November 13, 2024 19:47
@mabdinur mabdinur marked this pull request as ready for review November 13, 2024 21:12
@mabdinur mabdinur requested review from a team as code owners November 13, 2024 21:12
@mabdinur mabdinur requested review from christophe-papazian, dmehala, gnufede, jandro996 and smola and removed request for a team November 13, 2024 21:12
@mabdinur mabdinur changed the title parametric: Ensure all start span endpoints align with the spec parametric: Ensure start span endpoints align with the spec Nov 13, 2024
Comment thread utils/build/docker/php/parametric/server.php Outdated
Comment thread utils/build/docker/php/parametric/server.php Outdated
Comment thread utils/parametric/headers.py Outdated
@cbeauchesne
Copy link
Copy Markdown
Collaborator

From framework usage, AGTM. But to do a proper review, could you ask to someone familiar with the tested feature ?

Comment thread utils/build/docker/nodejs/parametric/server.js Outdated
Comment thread utils/build/docker/nodejs/parametric/server.js
Copy link
Copy Markdown
Contributor

@khanayan123 khanayan123 left a comment

Choose a reason for hiding this comment

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

Outside of a minor nit. LGTM

Comment thread utils/build/docker/nodejs/parametric/server.js Outdated
Copy link
Copy Markdown
Contributor

@ZStriker19 ZStriker19 left a comment

Choose a reason for hiding this comment

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

From a Python tracer and system-tests perspective looks great! Awesome work!

Comment thread utils/build/docker/golang/parametric/datadog.go
Copy link
Copy Markdown
Contributor

@mtoffl01 mtoffl01 left a comment

Choose a reason for hiding this comment

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

Fine from my side, although have a question about new spanContexts field on apmClientServer.

Copy link
Copy Markdown
Collaborator

@cbeauchesne cbeauchesne left a comment

Choose a reason for hiding this comment

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

Framework usage: all good.

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.

7 participants