Skip to content

chore(aggregator): implement telemetry for aggregator - #949

Closed
JuArce wants to merge 10 commits into
stagingfrom
936-chore-implement-opentelemetry-for-aggregator
Closed

chore(aggregator): implement telemetry for aggregator#949
JuArce wants to merge 10 commits into
stagingfrom
936-chore-implement-opentelemetry-for-aggregator

Conversation

@JuArce

@JuArceJuArce commented Sep 10, 2024•
edited
Loading

Copy link
Copy Markdown
Collaborator

Description

This PR allows the aggregator to export telemetry traces to have a better observability of the system

Tasks

  • Docker compose with Otel Collector and Jaeger UI
  • Init traces on NewTask
  • Add span on Operator responses
  • Add span on Quorum Reached
  • Add span on Tasks errors (ie task expired)
  • Close traces after a period of time the quorum was reached to wait for most operators responses

How to Test

Note

Docker is needed to test the PR.

  1. Run telemetry
make run_telemetry

It will run the Otel-Collector and Jaeger UI.

Note

Jaeger UI is available on http:localhost:16686

image
  1. Run anvil
make anvil_start_with_block_time
  1. Run aggregator
make aggregator_start
  1. Run operator
export OPERATOR_ADDRESS=0x70997970C51812dc3A010C7d01b50e0d17dc79C8
make operator_full_registration CONFIG_FILE=config-files/config-operator-1.yaml
make operator_start CONFIG_FILE=config-files/config-operator-1.yaml
  1. Run batcher
make batcher_start_local

  1. Send tasks

  2. Once you sent a batch and it has been responded by the aggregator, you should be able to check the trace of the task in the Jaeger UI (http:localhost:16686)

In the search, select service: aggregator and press find traces
It should show you the tasks you sent to aligned

taturosati
taturosati reviewed Sep 11, 2024
View reviewed changes
Comment threadaggregator/internal/pkg/aggregator.go Outdated
taturosati
taturosati reviewed Sep 11, 2024
View reviewed changes
Comment threadtelemetry/telemetry.go

@taturosatitaturosatiSep 11, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should probably add a TaskExpiredTrace when a task expires instead of reaching threshold, ideally should included quorum % and operators who responded if we have access to that information.

@taturosati

taturosati commented Sep 11, 2024

Copy link
Copy Markdown
Contributor

Other than comments, worked perfectly on my machine

@JuArceJuArce self-assigned this Sep 12, 2024
@JuArce
JuArce marked this pull request as ready for review September 12, 2024 21:25
uri-99
uri-99 suggested changes Sep 12, 2024
View reviewed changes

@uri-99uri-99 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Documentation of this PR would be highly valuable, maybe even with some screenshots.
(review still WIP)

@uri-99

uri-99 commented Sep 13, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor
image

This Warning seems to be appearing when the task was not yet responded, and it disappears afterwards. Can we change the text of the warning?

@uri-99

uri-99 commented Sep 13, 2024

Copy link
Copy Markdown
Contributor

Is there a way to make Jaeger auto-refresh?

@uri-99

uri-99 commented Sep 13, 2024

Copy link
Copy Markdown
Contributor

Have you considered setting up the /monitor ? Seems like it is a real-time visualization of the items found with the search.

image

uri-99
uri-99 reviewed Sep 13, 2024
View reviewed changes
Comment threadtelemetry/telemetry.go
uri-99
uri-99 reviewed Sep 13, 2024
View reviewed changes
Comment threadtelemetry/telemetry.go
attribute.String("merkle_root", fmt.Sprintf("0x%s", hex.EncodeToString(batchMerkleRoot[:]))),
attribute.String("status", "ok"),
),
) // TODO add quorum %

@uri-99uri-99Sep 13, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this TODO be resolved in this PR?

@JuArceJuArceSep 13, 2024

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

This will be added in another pr

uri-99
uri-99 reviewed Sep 13, 2024
View reviewed changes
Comment threadaggregator/internal/pkg/aggregator.go

@uri-99uri-99Sep 13, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you think about adding traces around lines 317 of this file? If the aggregator fails to SendAggregatedResponse after reaching quorum

@JuArceJuArce mentioned this pull request Sep 25, 2024
Merged
7 tasks
@JuArce

JuArce commented Sep 30, 2024

Copy link
Copy Markdown
CollaboratorAuthor

Superseded by #1077

@JuArceJuArce closed this Sep 30, 2024
@JuArce
JuArce deleted the 936-chore-implement-opentelemetry-for-aggregator branch September 30, 2024 15:25
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

4 more reviewers
@uri-99uri-99uri-99 requested changes
@taturosatitaturosatitaturosati left review comments
@PatStilesPatStilesPatStiles approved these changes
@entropidelicentropidelicentropidelic approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@JuArceJuArce

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@JuArce@taturosati@uri-99@PatStiles@entropidelic