Skip to content

Telemetry test fixes#1448

Merged
andrewlock merged 6 commits intomainfrom
andrew/telemetry-fixes
Aug 3, 2023
Merged

Telemetry test fixes#1448
andrewlock merged 6 commits intomainfrom
andrew/telemetry-fixes

Conversation

@andrewlock
Copy link
Copy Markdown
Member

Description

  • Removed TELEMETRY_MESSAGE_BATCH_EVENT_ORDER scenario
    • There are now no ordering requirements
  • Added basic (replacement) test for message batching to default scenario
    • Just asserts that message batching is being used
  • Fixed telemetry_app_started_products_disabled scenario
    • The scenario was not correctly disabling APP_SEC
    • The secenario was incorrectly asserting that no products object is provided on app-started
  • Added additional requirement on app-started with message-batch
    • app-started should be the first message in the first batch
  • Fixed using incorrect metrics variable
    • DD_TELEMETRY_METRICS_COLLECTION_ENABLED is not the correct variable

Motivation

After discussing with @mehulsonowal, this addresses a number of bugs and changes

Workflow

  1. ⚠️⚠️ Create your PR as draft
  2. Follow the style guidelines of this project (See how to easily lint the code)
  3. Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
  4. Mark it as ready for review

Once your PR is reviewed, you can merge it! ❤️

Reviewer checklist

  • Check what scenarios are modified. If needed, add the relevant label (run-parametric-scenario, run-profiling-scenario...). If this PR modifies any system-tests internal, then add the run-all-scenarios label (more info).
  • CI is green
    • If not, failing jobs are not related to this change (and you are 100% sure about this statement)
  • if any of build-some-image label is present
    1. is the image labl have been updated ?
    2. just before merging, locally build and push the image to hub.docker.com

- Reporting disabled products is acceptable in this scenario
- appsec_enabled must also be explicitly set otherwise it's not honoured
- Removed TELEMETRY_MESSAGE_BATCH_EVENT_ORDER scenario as decided it's not required
- Add test that message-batching is being used
The app-started event should be the first message in a message-batch
@andrewlock andrewlock force-pushed the andrew/telemetry-fixes branch from 04a031d to b0afe12 Compare July 31, 2023 13:40
@andrewlock andrewlock marked this pull request as ready for review July 31, 2023 15:53
@andrewlock andrewlock requested a review from a team as a code owner July 31, 2023 15:53
@andrewlock
Copy link
Copy Markdown
Member Author

AFAICT the failing tests are unrelated to this change (the python tests were failing previously, the otel tests are a 429 from the backend, and the node tests were dsm-related

Copy link
Copy Markdown
Collaborator

@pawelchcki pawelchcki left a comment

Choose a reason for hiding this comment

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

Looks good to me but - it could use a second look from @DataDog/telemetry-and-analytics

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants