Skip to content

Add support for message-batch (v2) payloads#1411

Merged
andrewlock merged 2 commits intomainfrom
andrew/add-support-for-message-batch-payload
Jul 20, 2023
Merged

Add support for message-batch (v2) payloads#1411
andrewlock merged 2 commits intomainfrom
andrew/add-support-for-message-batch-payload

Conversation

@andrewlock
Copy link
Copy Markdown
Member

Description

Add support for v2 telemetry message-batch payloads by "flattening" the batch into multiple requests. Not all tests require this, but many do.

Motivation

The telemetry tests commonly assert for particular message types, but when you use message-batch, the messages are nested inside the payload. So instead of this:

{
  "request": {
    "content": {
      "api_version": "v1",
      "request_type": "app-started",
      "payload": {}
    }
  }
},
{
  "request": {
    "content": {
      "api_version": "v1",
      "request_type": "app-dependencies-loaded",
      "payload": {}
    }
  }

you have something like this:

{
  "request": {
    "content": {
      "api_version": "v2",
      "request_type": "message-batch",
      "payload": [
        {
          "request_type": "app-started",
          "payload": {}
        },
        {
          "request_type": "app-dependencies-loaded",
          "payload": {}
        }
      ]
    }
  }
}

To handle that, added an extra helper function. As well as get_telemetry_data() there's now also get_flattened_telemetry_data()

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

  • If this PR modifies anything else than strictly the default scenario, 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

@andrewlock andrewlock marked this pull request as ready for review July 17, 2023 08:39
@andrewlock andrewlock requested a review from a team as a code owner July 17, 2023 08:39
Comment thread utils/interfaces/_library/core.py Outdated
def get_telemetry_data(self):
yield from self.get_data(path_filters="/telemetry/proxy/api/v2/apmtelemetry")

def get_flattened_telemetry_data(self):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

IIUC, we want to keep all test logics used in get_telemetry_data applied on sub-payloads if request_type == "message-batch".

What about using the same function, but with an optional argument, set by default to True ?

get_telemetry_data(self, flatten_message_batches=True) ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

IIUC, we want to keep all test logics used in get_telemetry_data applied on sub-payloads

Yes, in most cases. There are some cases (where we're operating at the "request" level rather than the "payload" level) where we don't want that logic though, hence adding this additional method.

What about using the same function, but with an optional argument, set by default to True

Yep, that works for me 🙂 I'll update to that approach instead 👍

Also, I've just realised that telemetry has some explicit additional scenarios, e.g. TELEMETRY_METRIC_GENERATION_DISABLED etc. I assume I should add the run-all-scenarios to this (after making the change)?

Thanks!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I assume I should add the run-all-scenarios to this (after making the change)?

yep :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done 🙂 There were some failing tests, but AFAICT they're probably flaky as they don't touch the telemetry stuff. Is it ok to just retry them? (Not sure of norms on this repo yet!)

@andrewlock andrewlock requested a review from a team as a code owner July 19, 2023 09:26
Add optional param to get_telemetry_data() instead.
Update previous usages of get_telemetry_data() that _shouldn't_ be flattened
@andrewlock andrewlock force-pushed the andrew/add-support-for-message-batch-payload branch from f034041 to e938fcb Compare July 19, 2023 10:10
@andrewlock andrewlock merged commit 0f4a5b7 into main Jul 20, 2023
@andrewlock andrewlock deleted the andrew/add-support-for-message-batch-payload branch July 20, 2023 12:03
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.

2 participants