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

Open
wants to merge 1 commit into
base: staging
Choose a base branch
from

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

@@ -285,10 +285,12 @@ func (agg *Aggregator) handleBlsAggServiceResponse(blsAggServiceResp blsagg.BlsA
if err == nil {
// In some cases, we may fail to retrieve the receipt for the transaction.
txHash := "Unknown"
effectiveGasPrice := "Unknown"
Copy link
Contributor

Choose a reason for hiding this comment

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

Very very much a nit but in a separate pr we should move these to a constants file.

@PatStiles
Copy link
Contributor

Ran locally and works well!

@JulianVentura JulianVentura self-assigned this Dec 10, 2024
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.

3 participants