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: add observability to anchoring worker #1199

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

samika98
Copy link
Contributor

[Replace Me With Meaningful Name] - #[Issue]

Description

Added spans to see how long each transaction while anchoring candidates takes in the worker.
This will help us identify bottlenecks and improve scalability.

How Has This Been Tested?

Describe the tests that you ran to verify your changes. Provide instructions for reproduction.

  • Test A (e.g. Test A - New test that ... ran in local, docker, and dev unstable.)
  • Test B

Definition of Done

Before submitting this PR, please make sure:

  • The work addresses the description and outcomes in the issue
  • I have added relevant tests for new or updated functionality
  • My code follows conventions, is well commented, and easy to understand
  • My code builds and tests pass without any errors or warnings
  • I have tagged the relevant reviewers
  • I have updated the READMEs of affected packages
  • I have made corresponding changes to the documentation
  • The changes have been communicated to interested parties

References:

Please list relevant documentation (e.g. tech specs, articles, related work etc.) relevant to this change, and note if the documentation has been updated.

@samika98 samika98 requested a review from smrz2001 April 26, 2024 19:57
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (feat/no-ipfs@9b834fc). Click here to learn what that means.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@               Coverage Diff               @@
##             feat/no-ipfs    #1199   +/-   ##
===============================================
  Coverage                ?   81.70%           
===============================================
  Files                   ?       46           
  Lines                   ?     1847           
  Branches                ?      291           
===============================================
  Hits                    ?     1509           
  Misses                  ?      338           
  Partials                ?        0           

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

@@ -302,45 +302,65 @@ export class AnchorService {
private async _anchorCandidates(candidates: Candidate[]): Promise<Partial<AnchorSummary>> {
logger.imp(`Creating Merkle tree from ${candidates.length} selected streams`)
const span = Metrics.startSpan('anchor_candidates')

const buildMerkleTreeSpan = Metrics.startSpan('build_merkle_tree')

Choose a reason for hiding this comment

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

Does startSpan create timing metrics automatically? As in, how long a given span takes on avg, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A span represents a single operation within a specific trace. Traces are used to monitor the execution paths in a system. startSpan() will handle timing by reporting start and endTime.
It does not allow us to do aggregations, like counts or gauges. So the answer to your question is : no https://opentelemetry.io/docs/concepts/signals/traces/#spans

Choose a reason for hiding this comment

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

In that case, can we please add metrics for the time spent in each of those spans?

Counter for how many time a span is taken, a summary for how long each took, etc.

Copy link
Contributor Author

@samika98 samika98 May 10, 2024

Choose a reason for hiding this comment

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

I am wondering if this is necessary after we data-dog profiling enabled on CAS @christianlavoie @smrz2001

Base automatically changed from feat/no-ipfs to develop May 2, 2024 19:41
@samika98 samika98 requested a review from gvelez17 May 7, 2024 16:44
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