[Feat][DEVX-364] Record Custom Metric (Response Time) #861

ajimae · 2024-11-27T00:00:37Z

Summary

Record custom metrics (request-response time)

Completed Tasks

add a helper to get individual request start and end times
add APM agents to record and send custom metrics to respective APMs

Depends On

#877

changeset-bot · 2024-11-27T00:00:42Z

🦋 Changeset detected

Latest commit: 9d1df0b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@commercetools/ts-sdk-apm	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codecov · 2024-11-27T00:23:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.05%. Comparing base (1f119a4) to head (9d1df0b).
Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #861   +/-   ##
=======================================
  Coverage   93.05%   93.05%           
=======================================
  Files          25       25           
  Lines         288      288           
  Branches       14       14           
=======================================
  Hits          268      268           
  Misses         20       20

Flag	Coverage Δ
integrationtests	`93.05% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- record and send custom metrics to respective APMs

- fix tests for old (legacy) apm middleware

- improve typescript types for ts-sdk-apm package - add more custom metric example snippets to example directory - add tests to validate functionality

- add release changeset

- add node-fetch dependency to apm package

- rebase and update branch - move test to integration tests directory - update dependencies

lojzatran

Good job 👍

examples/newrelic-express-apm/package.json

packages/ts-sdk-apm/src/apm-legacy.ts

lojzatran · 2025-01-14T11:16:57Z

examples/newrelic-express-apm/src/controller/ProductController.js

      return ResponseHandler.successResponse(
        res,
        data.statusCode || data.body.statusCode,
        data.message || data.body.message,
        data.body
      )
    }
+
+    const end = performance.now()
+    newrelic.recordMetric('Commercetools/Test/Response/Time/Total', end - start)


You have duplicated code.

btw. should this be recorded inside the ts-apm-sdk? There is a metric Commercetools/Client/Response/Total already. Why would the user need the second metric here?

Uhmmm not necessarily a duplicate. What I was looking at is how the custom can instrument an individual function in their codebase. As you may have seen, the only thing we recorded in the telemetryMiddleware was the response time. However with this, they can record any other metric include a status code, execution time of a function etc.

Sorry, I was not clear. By duplicated I mean this part is twice. It could be done before if once:

const end = performance.now() newrelic.recordMetric( 'Commercetools/Test/Response/Time/Total', end - start )

If you want to show a custom metric created by user, then try to use a different name and don't start with Commercetools. I'm also not sure if we should provide newrelic in our package. This should be the responsibility of the user's app to use newrelic dependency directly. Instead we could add some more metrics to the telemetryMiddleware, like count of errors and success requests. WDYT?

That is why we have a separate package for telemetry. I think the newrelic and dd-trace dependencies are both absolutely necessary here and should be part of the offering. These dependencies shouldn't be coming from the customer's application.

Also, you cannot totally account for all the possible scenario where or what the customer would want to instrument. I would suggest giving them the freedom to record any metric of their choice, except you want us to limit what they can monitor?

OK, I see your point. I find it a bit weird that we offer also newrelic, datadog etc. when the customer could actually use the original newrelic or datadog packages by themselves.
We do not add anything extra to it, so I'm wondering why would the customer wants to use newrelic from ts-sdk-apm package if they just want to use newrelic for their custom metrics? Would it be better for them to use directly the package from newrelic directly?

examples/newrelic-express-apm/src/utils/response.js

packages/ts-sdk-apm/src/apm.ts

packages/ts-sdk-apm/src/helpers.ts

packages/ts-sdk-apm/src/index.ts

packages/platform-sdk/test/integration-tests/sdk-v3/telemetry.test.ts

examples/newrelic-express-apm/src/sdk-v2/sdk.js

packages/ts-sdk-apm/test/apm.test/apm.test.ts

- implement PR review feedback

- implement feedback

barbara79

LGTM 👍

…-metrics

ajimae requested a review from a team as a code owner November 27, 2024 00:00

ajimae marked this pull request as draft November 27, 2024 00:01

ajimae changed the title ~~[Feat][Dev-364] Record Custom Metric (Response Time)~~ [Feat][DEVX-364] Record Custom Metric (Response Time) Nov 27, 2024

ajimae force-pushed the feat/add-requests-response-time-and-custom-metrics branch from e834325 to 5f7d9bb Compare November 27, 2024 00:17

ajimae marked this pull request as ready for review November 27, 2024 15:36

ajimae force-pushed the feat/add-requests-response-time-and-custom-metrics branch from b63f0b8 to 8fb70b5 Compare November 27, 2024 15:39

ajimae marked this pull request as draft November 27, 2024 15:54

ajimae added 5 commits January 13, 2025 01:33

feat(custom-metric): record custom metric

f33db96

- record and send custom metrics to respective APMs

chore(tests): fix tests

ac5ef8b

- fix tests for old (legacy) apm middleware

chore(ts-sdk-apm): improve package

6777f90

- improve typescript types for ts-sdk-apm package - add more custom metric example snippets to example directory - add tests to validate functionality

chore(release-changeset): release changeset

24037e6

- add release changeset

chore(ts-sdk-apm): add missing package

76f93c8

- add node-fetch dependency to apm package

ajimae force-pushed the feat/add-requests-response-time-and-custom-metrics branch from 6e60c49 to bcbbc6a Compare January 13, 2025 02:48

chore(custom-metrics): rebase and update branch

609a0c9

- rebase and update branch - move test to integration tests directory - update dependencies

ajimae force-pushed the feat/add-requests-response-time-and-custom-metrics branch from bcbbc6a to 609a0c9 Compare January 13, 2025 02:53

ajimae marked this pull request as ready for review January 13, 2025 03:08

ajimae requested review from lojzatran and barbara79 January 13, 2025 03:08

lojzatran reviewed Jan 14, 2025

View reviewed changes

barbara79 reviewed Jan 14, 2025

View reviewed changes

packages/platform-sdk/test/integration-tests/sdk-v3/telemetry.test.ts Outdated Show resolved Hide resolved

barbara79 reviewed Jan 14, 2025

View reviewed changes

examples/newrelic-express-apm/src/sdk-v2/sdk.js Outdated Show resolved Hide resolved

barbara79 reviewed Jan 14, 2025

View reviewed changes

packages/ts-sdk-apm/test/apm.test/apm.test.ts Outdated Show resolved Hide resolved

ajimae added 3 commits January 14, 2025 15:44

chore(ts-sdk-apm): implement feedback

6b0cdd5

- implement PR review feedback

chore(ts-sdk-apm): implement feedback

e575192

- implement PR review feedback

chore(ts-sdk-apm): implement feedback

4492634

- implement feedback

barbara79 approved these changes Jan 15, 2025

View reviewed changes

lojzatran approved these changes Jan 15, 2025

View reviewed changes

Merge branch 'master' into feat/add-requests-response-time-and-custom…

9d1df0b

…-metrics

ajimae merged commit e8a0e92 into master Jan 15, 2025
6 checks passed

ajimae deleted the feat/add-requests-response-time-and-custom-metrics branch January 15, 2025 09:48

ct-changesets bot mentioned this pull request Jan 15, 2025

Version Packages #901

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feat][DEVX-364] Record Custom Metric (Response Time) #861

[Feat][DEVX-364] Record Custom Metric (Response Time) #861

ajimae commented Nov 27, 2024 •

edited

Loading

changeset-bot bot commented Nov 27, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading

lojzatran left a comment

lojzatran Jan 14, 2025

ajimae Jan 14, 2025

lojzatran Jan 14, 2025

lojzatran Jan 14, 2025

ajimae Jan 14, 2025 •

edited

Loading

lojzatran Jan 15, 2025

barbara79 left a comment

[Feat][DEVX-364] Record Custom Metric (Response Time) #861

[Feat][DEVX-364] Record Custom Metric (Response Time) #861

Conversation

ajimae commented Nov 27, 2024 • edited Loading

Summary

Completed Tasks

Depends On

changeset-bot bot commented Nov 27, 2024 • edited Loading

🦋 Changeset detected

codecov bot commented Nov 27, 2024 • edited Loading

Codecov Report

lojzatran left a comment

Choose a reason for hiding this comment

lojzatran Jan 14, 2025

Choose a reason for hiding this comment

ajimae Jan 14, 2025

Choose a reason for hiding this comment

lojzatran Jan 14, 2025

Choose a reason for hiding this comment

lojzatran Jan 14, 2025

Choose a reason for hiding this comment

ajimae Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

lojzatran Jan 15, 2025

Choose a reason for hiding this comment

barbara79 left a comment

Choose a reason for hiding this comment

ajimae commented Nov 27, 2024 •

edited

Loading

changeset-bot bot commented Nov 27, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading

ajimae Jan 14, 2025 •

edited

Loading