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: Improve jaeger traces on the aggregator #1603

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

JulianVentura
Copy link
Collaborator

Improve jaeger traces on the aggregator

Description

This PR makes two modifications on the event traces generated while responding to a task on the aggregator:

  1. Removed the Bump gas price event, in favor of Gas price set so we not only get information of the used gas prices when a bump occurs, but also on the first attempt.
  2. Added the effective_gas_price attribute to the Task Sent to Ethereum event, so we know exactly what was the gas price fee after sending the transaction (which may be lower than the one specified, see EIP-1559)
  3. Removed a LogTaskError on sendAggregatedResponse as it was being duplicated

How to test

First test, normal behavior

  1. Start all services, including telemetry and metrics (grafana dashboard)
  2. Send a task, with make batcher_send_sp1_task
  3. Wait for the task to be verified
  4. You should see that the dashboard "# Bumps Sending Responses to Ethereum" is at zero.
  5. You should see on jaeger these events:
image

Second test, retry behavior

  1. Simulate a retry in the aggregator by adding the following to the avs_writer.go
178	if i < 3 {
	    i++
	    return nil, fmt.Errorf("My error")
        }
  1. Start all services, including telemetry and metrics (grafana dashboard)
  2. Send a task, with make batcher_send_sp1_task
  3. You should see that the dashboard "# Bumps Sending Responses to Ethereum" is now indicating 3 bumps
  4. You should see on jaeger these events:
image

Type of change

Please delete options that are not relevant.

  • New feature
  • Bug fix
  • Optimization
  • Refactor

Checklist

  • “Hotfix” to testnet, everything else to staging
  • Linked to Github Issue
  • This change depends on code or research by an external entity
    • Acknowledgements were updated to give credit
  • Unit tests added
  • This change requires new documentation.
    • Documentation has been added/updated.
  • This change is an Optimization
    • Benchmarks added/run
  • Has a known issue
  • If your PR changes the Operator compatibility (Ex: Upgrade prover versions)
    • This PR adds compatibility for operator for both versions and do not change batcher/docs/examples
    • This PR updates batcher and docs/examples to the newer version. This requires the operator are already updated to be compatible

aggregator/pkg/aggregator.go Show resolved Hide resolved
@PatStiles
Copy link
Contributor

Ran locally and works well!

@JulianVentura JulianVentura self-assigned this Dec 10, 2024
@JuArce JuArce added this pull request to the merge queue Dec 23, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 23, 2024
@JuArce JuArce added this pull request to the merge queue Dec 23, 2024
Merged via the queue into staging with commit 9ca55fb Dec 23, 2024
3 checks passed
@JuArce JuArce deleted the feat/aggregator-improve-traces branch December 23, 2024 16:23
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