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

Feat/performance tracking #1896

Merged
merged 30 commits into from
Nov 26, 2024
Merged

Feat/performance tracking #1896

merged 30 commits into from
Nov 26, 2024

Conversation

gsteenkamp89
Copy link
Contributor

@gsteenkamp89 gsteenkamp89 commented Nov 7, 2024

closes ACX-3198

Goal:
Provide a standardised way of logging performance metrics.
The API is similar to using Node's native perf_hooks, but slightly easier to use.

The Profiler will ensure { datadog: true } is appended to all logs so we can ingest this by searching for this key.

Since the Profiler class holds some state, each instance should be created within a function scope so it can be garbage collected.

async function functionToMeasure( logger: Logger ) {
   const profiler = new Profiler({
      logger,
      at: "Relayer::functionToMeasure"
   })

  // drop first mark
  profiler.mark("A");

  // do stuff
  await step1();

  profiler.mark("B", { depositId: 123 });

  // do stuff
  await step2();

  profiler.mark("C", { relayerAddress: "0xabcabc1231231" });

  // Measure durations between marks
  profiler.measure("task1", {
    from: "A",
    to: "B",
    message: "Time from task A to task B",
  });

  profiler.measure("task2", {
    from: "B",
    to: "C",
    message: "Time from task B to task C",
  });
  profiler.measure("Total", {
    from: "A",
    to: "C",
    message: "Time from task A to task C",
  });
}

or wrap an async function with measureAsync

async function calculateFoo(...args: any): Promise<Foo> {
   return Foo
}

const foo = await profiler.measureAsync(calculateFoo(...args), "calculateFoo", {
   message: "Time taken to calculate Foo",
   ...args // log additional data
});

Copy link
Contributor

@pxrl pxrl left a comment

Choose a reason for hiding this comment

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

Great to see this - I left a few comments; interested to see what you think.

src/utils/LogUtils.ts Outdated Show resolved Hide resolved
src/utils/LogUtils.ts Outdated Show resolved Hide resolved
src/utils/LogUtils.ts Outdated Show resolved Hide resolved
@gsteenkamp89 gsteenkamp89 marked this pull request as ready for review November 8, 2024 14:36
Copy link

linear bot commented Nov 8, 2024

src/utils/LogUtils.ts Outdated Show resolved Hide resolved
Copy link
Member

@nicholaspai nicholaspai left a comment

Choose a reason for hiding this comment

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

where is the logic for this?

The Profiler will ensure { datadog: true } is appended to all logs

This looks like a great approach, I like the class' interface a lot

@gsteenkamp89
Copy link
Contributor Author

where is the logic for this?

The Profiler will ensure { datadog: true } is appended to all logs

This looks like a great approach, I like the class' interface a lot

the default metadata { datadog: true } is being set here

src/utils/LogUtils.ts Outdated Show resolved Hide resolved
src/utils/LogUtils.ts Outdated Show resolved Hide resolved
src/relayer/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@pxrl pxrl left a comment

Choose a reason for hiding this comment

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

@gsteenkamp89 LGTM. Pending that one question on the logger, are you OK to re-propose Profiler in the SDK? Then we can keep this PR open and use it to update/align all the uses of the profiler in the relayer.

src/utils/Profiler.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@james-a-morris james-a-morris left a comment

Choose a reason for hiding this comment

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

Is there any overlap here? across-protocol/indexer#95

@gsteenkamp89
Copy link
Contributor Author

gsteenkamp89 commented Nov 19, 2024

Is there any overlap here? across-protocol/indexer#95

Oh jeez.. Yes there's definitely overlap. Although looking at David's PR, it seems the goal there as slightly different maybe? This profiler is just logging production performance data to datadog.
The benchmarker is generating reports on demand or on a schedule which is maybe more useful in a CI job as a part of testing our code in the development cycle.
🤔

yarn.lock Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/utils/index.ts Outdated Show resolved Hide resolved
@gsteenkamp89 gsteenkamp89 requested a review from pxrl November 19, 2024 16:45
src/relayer/index.ts Outdated Show resolved Hide resolved
@pxrl pxrl merged commit 5bf9721 into master Nov 26, 2024
4 checks passed
@pxrl pxrl deleted the feat/performance-tracking branch November 26, 2024 10:12
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.

5 participants