Skip to content
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

Scott.solmonson/batch item failure metric #532

Merged
merged 13 commits into from
Apr 25, 2024

Conversation

ssolmonson
Copy link
Contributor

@ssolmonson ssolmonson commented Apr 19, 2024

What does this PR do?

Creates a metric for batch item failures and closes issue #435

Motivation

#435

Testing Guidelines

  • Added unit tests
  • Tested the changes manually

Additional Notes

SVLS-3126

Types of Changes

  • Bug fix
  • New feature
  • Breaking change
  • Misc (docs, refactoring, dependency upgrade, etc.)

Check all that apply

  • This PR's description is comprehensive
  • This PR contains breaking changes that are documented in the description
  • This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
  • This PR impacts documentation, and it has been updated (or a ticket has been logged)
  • This PR's changes are covered by the automated tests
  • This PR collects user input/sensitive content into Datadog
  • This PR passes the integration tests (ask a Datadog member to run the tests)

@ssolmonson ssolmonson requested a review from a team as a code owner April 19, 2024 22:04
Copy link
Contributor

@astuyve astuyve left a comment

Choose a reason for hiding this comment

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

This PR is a really good first stab at this feature, well done @scott.solmonson!

I've added a few notes for your reference, but I'll pick up this PR and get it shipped.

The biggest thing is that this implementation sets a value of 1 if batchItemFailures is present. We'd like this to be the count of failures.

Otherwise, this looks pretty good with some minor style tweaks.

Thanks again!

@@ -0,0 +1,12 @@
export interface LambdaResponse {
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't needed. This file should be just a utils file for batch item failures, it doesn't cover all responses (nor does it need to)

batchItemFailures?: { itemIdentifier: string }[];
}

export function isBatchItemFailure(lambdaResponse: any): lambdaResponse is LambdaResponse {
Copy link
Contributor

Choose a reason for hiding this comment

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

lambdaResponse is LambdaResponse is not needed here, not sure what it's trying to do but this function returns a boolean.

@@ -81,3 +81,7 @@ export function incrementInvocationsMetric(listener: MetricsListener, context: C
export function incrementErrorsMetric(listener: MetricsListener, context: Context): void {
incrementEnhancedMetric(listener, "errors", context);
}

export function incrementBatchItemFailureMetric(listener: MetricsListener, context: Context): void {
incrementEnhancedMetric(listener, "batch_item_failures", context);
Copy link
Contributor

Choose a reason for hiding this comment

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

This sets 1 for all batch item failures. We'd want this to be a count of the failures.

@@ -2,26 +2,35 @@ import http from "http";
import nock from "nock";

import { Context, Handler } from "aws-lambda";

Copy link
Contributor

Choose a reason for hiding this comment

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

generally we try not to introduce new whitespace changes unless the original violated whitespace rules

Copy link
Contributor

@duncanista duncanista left a comment

Choose a reason for hiding this comment

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

LGTM – great tests and approach.

src/metrics/enhanced-metrics.ts Outdated Show resolved Hide resolved
src/utils/response.ts Outdated Show resolved Hide resolved
@astuyve astuyve merged commit 1621e81 into main Apr 25, 2024
25 checks passed
@astuyve astuyve deleted the scott.solmonson/batch-item-failure-metric branch April 25, 2024 17:35
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.

3 participants