Skip to content

Conversation

@astuyve
Copy link
Contributor

@astuyve astuyve commented Aug 26, 2025

What does this PR do?

This should probably be a test macro but I kinda like a no-macro rule generally so I'm just adding this method back so we can assert on things we put in the aggregator.

Motivation

Additional Notes

Describe how to test/QA your changes

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a get_entry_by_id command to the aggregator service to enable fetching metrics by their identifier for testing purposes. This allows test code to verify that metrics have been properly stored in the aggregator.

  • Adds a new GetEntryById command variant to the AggregatorCommand enum
  • Implements the get_entry_by_id method on AggregatorHandle for external access
  • Adds command handling logic in the service loop to process get requests
  • Includes comprehensive tests to verify the new functionality works correctly

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +141 to +142
if let Err(_) = response_tx.send(response) {
error!("Failed to send get_entry_by_id response - receiver dropped");
Copy link

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

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

[nitpick] The error value is being ignored with _. Consider logging the error or using a more descriptive variable name like _send_error to clarify the intentional discard.

Suggested change
if let Err(_) = response_tx.send(response) {
error!("Failed to send get_entry_by_id response - receiver dropped");
if let Err(send_error) = response_tx.send(response) {
error!("Failed to send get_entry_by_id response - receiver dropped: {:?}", send_error);

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nope

@litianningdatadog litianningdatadog merged commit abfec75 into main Aug 28, 2025
26 checks passed
@litianningdatadog litianningdatadog deleted the aj/get-metric-by-id branch August 28, 2025 20:07
litianningdatadog pushed a commit to DataDog/datadog-lambda-extension that referenced this pull request Aug 29, 2025
1. Upgrades dogstatsd-rs so that we use the channel based, lock-free
aggregator
2. sets the context size to 500 contexts instead of 5, which is helpful
for very very fast functions

TODO update git sha when we merge
DataDog/serverless-components#38
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