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(eth): introduce output type for passing full event msgs #40

Merged
merged 8 commits into from
Nov 28, 2024

Conversation

mattevans
Copy link
Contributor

@mattevans mattevans commented Nov 26, 2024

👋 Hey team, it was great to meet some of you at DC7.

Xatu consumes Hermes events via the DataStreamTypeCallback and would really benefit in having the entire event payload passed-through. This PR introduces the concept of an DataStreamOutputType and implements Kinesis and Full renderers for each.

  • Kinesis event structure/output is unchanged.
  • StdLogger event structure/output is unchanged.
  • Callback event structure/output does change; using qualified types over map[string]any for its event payload data.
Running Locally
go run ./cmd/hermes --data.stream.type=callback eth \
  --chain=devnet \
  --devp2p.host=0.0.0.0 \
  --libp2p.host=0.0.0.0 \
  --prysm.host=127.0.0.1 \
  --prysm.port.http=33001 \
  --prysm.port.grpc=33003 \
  --genesis.ssz.url=http://127.0.0.1:40000/network-configs/genesis.ssz \
  --config.yaml.url=http://127.0.0.1:40000/network-configs/config.yaml \
  --bootnodes.yaml.url=http://127.0.0.1:40000/network-configs/boot_enr.yaml \
  --deposit-contract-block.txt.url=http://127.0.0.1:40000/network-configs/deposit_contract_block.txt

* main:
  build(deps): bump github.com/golang-jwt/jwt/v4 from 4.5.0 to 4.5.1
@mattevans mattevans marked this pull request as ready for review November 27, 2024 02:11
Copy link
Contributor

@cortze cortze left a comment

Choose a reason for hiding this comment

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

Thanks @mattevans for the PR. overall, LGTM, nice and clean. I just have one peaky comment that could even be ignored.

host/flush_tracer.go Show resolved Hide resolved
@dennis-tra
Copy link
Contributor

@mattevans looks super clean, thanks! Feel free to merge 👍

Just one suggestion (not blocking the merge), the DataStream interface could expose the output renderer directly like:

type DataStream interface {
	Start(ctx context.Context) error
	Stop(ctx context.Context) error
	PutRecord(ctx context.Context, event *TraceEvent) error
	Type() DataStreamType
	EventRenderer() DataStreamRenderer // or called something else
}

This would avoid the indirection of the output type switch.

@codecov-commenter
Copy link

codecov-commenter commented Nov 27, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 45.37815% with 390 lines in your changes missing coverage. Please review.

Project coverage is 9.44%. Comparing base (b94d32c) to head (73dc613).
Report is 164 commits behind head on main.

Files with missing lines Patch % Lines
eth/pubsub.go 0.00% 242 Missing ⚠️
eth/output_kinesis.go 72.17% 63 Missing and 6 partials ⚠️
eth/output_full.go 69.75% 62 Missing ⚠️
tele/tele.go 0.00% 9 Missing ⚠️
eth/reqresp.go 0.00% 2 Missing ⚠️
host/callback.go 0.00% 2 Missing ⚠️
host/kinesis.go 0.00% 2 Missing ⚠️
host/trace_logger.go 0.00% 2 Missing ⚠️

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

Additional details and impacted files
@@           Coverage Diff            @@
##            main     #40      +/-   ##
========================================
+ Coverage   7.75%   9.44%   +1.69%     
========================================
  Files          7      34      +27     
  Lines        400    4679    +4279     
========================================
+ Hits          31     442     +411     
- Misses       361    4220    +3859     
- Partials       8      17       +9     

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

@mattevans
Copy link
Contributor Author

Just one suggestion (not blocking the merge), the DataStream interface could expose the output renderer directly

Thanks 🙌

I think if I did this, it would mean the host pkg then has a dep on the eth pkg, which I was trying to avoid. 🤔


Really appreciate the such prompt reviews @dennis-tra @cortze. It looks like there's a few workflows awaiting approval before it can be merged. I don't have perms to kick them off.

@cortze cortze merged commit a39cba4 into probe-lab:main Nov 28, 2024
4 checks passed
@mattevans mattevans deleted the feat/output-transform branch November 28, 2024 08:54
@dennis-tra
Copy link
Contributor

I think if I did this, it would mean the host pkg then has a dep on the eth pkg, which I was trying to avoid. 🤔

Oh, of course! Yeah, let's avoid that. Thanks for pointing this out 👍

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