feat(traces): OTeL Traces implementation(duty flow) #1980
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oleg-ssvlabs
changed the title
oleg-ssvlabs
force-pushed
the
traces
branch
6 times, most recently
from
34dc244 to
819f619
Compare
oleg-ssvlabs
force-pushed
the
traces
branch
2 times, most recently
from
c79a8cc to
f63cc5e
Compare
oleg-ssvlabs
requested review from
moshe-blox,
y0sher,
zktaiga and
MatusKysel
* chore(networkconfig): add new SSV Labs bootnodes * Update mainnet.go * Update holesky.go
…tivation (#1689) * fix: (EventHandler) update non-committee shares upon liquidation/reactivation --------- Co-authored-by: olegshmuelov <[email protected]>
Co-authored-by: Matus Kysel <[email protected]> Co-authored-by: rehs0y <[email protected]>
…adata sync (#1805) * rename setupEventHandling to syncContractEvents * refactor beaconprotocol.UpdateValidatorsMetadata to MetadataFetcher * don't pass logger to operatorNode.Start() * don't pass logger to reportOperators * don't pass logger to p2pNetwork Setup and Start * fix TestSetupValidatorsExporter * reduce errors text in fetchAndUpdateValidatorsMetadata * extract a package for metadata updating; start it before p2p setup * minor cleanup * pass context * get rid of update metadata loop in validator controller * remove unused code from validator controller * fix tests for StartValidators * initialize metadata updater before validator controller * various fixes in metadata updater * sharesStorage -> shareStorage * remove redundant comment * avoid blocking on channel send * return shares instead of nil on timeout * fix TODO's; add tests * fix linter * review comments and some code improvements * review comments [2] * minor improvements * move metadata updater inside validator * review comments [3] * add comments * add another comment * network/p2p: extract logger changes to another PR * network/p2p: revert leftovers * resolve a busy loop * remove logic with indices diff * wrap context in reportIndicesChange * review comments * ValidatorSyncer -> Syncer * fix comment * rename receiver * get rid of fetcher * fix TestUpdateValidatorMetadata * NewValidatorSyncer -> NewSyncer * minor renames * add a comment in HandleMetadataUpdates * revert removal of active index comparison * add self subnets logic missed on merge conflicts * fix leftovers * apply changes from #1969 * filter shares by own subnets * use fixed subnets * improve the last batch sleep comment * minor rename * comments * comment * comment * logs --------- Co-authored-by: zippoxer <[email protected]>
Co-authored-by: moshe-blox <[email protected]> Co-authored-by: Matus Kysel <[email protected]>
zktaiga
reviewed
Feb 27, 2025
| cmd.Parent().Short, | ||
| cmd.Parent().Version, | ||
| observability.WithMetrics()) | ||
| observability.WithMetrics(), | ||
| observability.WithTraces("stage-alloy.alloy.svc:4317", true)) |
There was a problem hiding this comment.
Despite this being a stale PR that we'll soon revive, this should be configured via env vars.
There was a problem hiding this comment.
I'll take it in consideration
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description/Questions/Suggestions
Are both logs and events needed for recording of the same type of event? Should it be either-or? Example.
Is
Duty IDas a Trace Attribute necessary? Currently bothSlotandEpochattributes are added separately. Would it be better to just add aCommitteeattribute instead?Duty flows are separated into
Committeeversus everything else (method/function level). This is reflected in different span names. Should we keep separating them on a span level, or should we use the same names with attributes that help differentiate duties (likessv.runner.role,etc)Span name examples:
ssv.validator.execute_committee_dutyssv.validator.execute_dutyssv.validator.start_committee_dutyssv.validator.start_dutyThere are three statuses for spans:
Ok,Error, andUnset. Ideally, all spans should have their status explicitly set to eitherOkorError—Unsetis not expected. Please use Grafana UI and verify if we receive any of the spans inUnsetstatus.Look into namespaces for metrics, traces, and attributes. Namespaces should be consistent across all observability "primitives". There is a chance we have some inconsistencies
Example:
ssv.validator.dutyvs.ssv.duty. IfDutybelongs toValidator, usessv.validator.dutyeverywhere.OpenTelemetry Specification explains why
OKstatus is used without a message. ("Description MUST be IGNORED for StatusCode Ok & Unset values."). Even if the message is set for OK statuses, it will be ignored by OTeL and not displayed in Grafana (yeah, SDKs could have been better here)Some libraries needs to be updated for proper context propagation, especially for methods that perform I/O (e.g., HTTP calls). Example:
p2p.Broadcast().Some enums in the libraries used by SSV Node lack a
String()method, which complicates logging and tracing.Example:
types.PartialSigMsgType(ssv-speclib). Something that should potentially be implemented by these libraries (we own source code)Should these enums be moved to the
spec typespackage instead?