Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

jaeger integration to debug parachains #2047

Merged
merged 59 commits into from
Dec 11, 2020
Merged

jaeger integration to debug parachains #2047

merged 59 commits into from
Dec 11, 2020

Conversation

drahnr
Copy link
Contributor

@drahnr drahnr commented Dec 1, 2020 •
edited
Loading

Continuation of #2027

Goal: Integration of jaeger as part of the polkadot subsystems.

  1. more jaeger tracing in polkadot for more subsystems
  2. make the naming of spans consistent
  3. debug why tests need to start with --no-mdns since that triggers mDNS shouldn't panic if response packet becomes too large libp2p/rust-libp2p#1872
  4. integrate optionally in substrate, and use that instance this needs some more thoughts of what our goals are here

@drahnr drahnr self-assigned this Dec 1, 2020
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Dec 1, 2020
@drahnr drahnr added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. and removed A0-please_review Pull request needs code review. labels Dec 1, 2020
@drahnr drahnr marked this pull request as draft December 1, 2020 12:46
@coriolinus coriolinus changed the title gaeger integration to debug parachains jaeger integration to debug parachains Dec 1, 2020
@tomaka tomaka mentioned this pull request Dec 2, 2020
Closed
@drahnr drahnr marked this pull request as ready for review December 3, 2020 14:48
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Dec 3, 2020
@drahnr drahnr requested a review from ordian December 3, 2020 14:51
@drahnr drahnr requested a review from coriolinus December 7, 2020 14:19
ordian
ordian reviewed Dec 7, 2020
View reviewed changes
scripts/adder-collator.sh Outdated Show resolved Hide resolved
ordian
ordian reviewed Dec 7, 2020
View reviewed changes
node/subsystem/src/jaeger.rs Outdated Show resolved Hide resolved
ordian
ordian reviewed Dec 7, 2020
View reviewed changes
node/subsystem/src/jaeger.rs Outdated Show resolved Hide resolved
@ordian
Copy link
Member

ordian commented Dec 7, 2020

I think this is fine as PoC, but would be nice to create a ticket and link it in the docs for

this needs some more thoughts of what our goals are here

I assume whether we want to integrate it in substrate #2027 (comment) depends on it?

bkchr
bkchr reviewed Dec 10, 2020
View reviewed changes
cli/src/cli.rs Outdated Show resolved Hide resolved
node/core/proposer/src/lib.rs Outdated Show resolved Hide resolved
node/network/bitfield-distribution/src/lib.rs Outdated Show resolved Hide resolved
node/service/src/lib.rs Outdated Show resolved Hide resolved
node/subsystem/src/jaeger.rs Outdated Show resolved Hide resolved
node/subsystem/src/jaeger.rs Outdated Show resolved Hide resolved
drahnr and others added 6 commits December 10, 2020 14:19
@drahnr
comment cli --jaeger-agent option
9980e3e
@drahnr
don't bring down a node in case of hash with 16 leading zer0 bytes
9efc7dc
@drahnr @bkchr
white space wars
b48936d 
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
@drahnr @bkchr
white space wars
2b1f2b9 
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
@drahnr
avoid loop, use port 0 as rx port
27deffa 
OS picks a port in case of port 0 as rx port.
@drahnr @bkchr
white space wars
38d1a5b 
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
bkchr
bkchr reviewed Dec 10, 2020
View reviewed changes
cli/src/cli.rs Outdated Show resolved Hide resolved
bkchr
bkchr reviewed Dec 10, 2020
View reviewed changes
node/subsystem/src/jaeger.rs Outdated Show resolved Hide resolved
node/subsystem/src/jaeger.rs Outdated Show resolved Hide resolved
node/subsystem/src/jaeger.rs Outdated Show resolved Hide resolved
@drahnr
Copy link
Contributor Author

drahnr commented Dec 10, 2020

Must be squashed!

drahnr
drahnr commented Dec 10, 2020
View reviewed changes
node/subsystem/src/jaeger.rs Outdated Show resolved Hide resolved
@ordian
Copy link
Member

ordian commented Dec 11, 2020

@bkchr are you ok with merging this?

bkchr
bkchr approved these changes Dec 11, 2020
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

One last point that needs to be addressed before merging! Otherwise it is okay for now.

node/subsystem/src/jaeger.rs Outdated Show resolved Hide resolved
@drahnr drahnr force-pushed the bernhard-moar-tracing branch from f4c2497 to 0fac683 Compare December 11, 2020 15:54
bkchr
bkchr reviewed Dec 11, 2020
View reviewed changes
node/subsystem/src/jaeger.rs Outdated Show resolved Hide resolved
@drahnr drahnr merged commit b892796 into master Dec 11, 2020
@drahnr drahnr deleted the bernhard-moar-tracing branch December 11, 2020 16:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rphmeier rphmeier rphmeier left review comments

@coriolinus coriolinus coriolinus approved these changes

@bkchr bkchr bkchr approved these changes

@ordian ordian ordian approved these changes

Assignees

@drahnr drahnr

Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@drahnr @ordian @bkchr @coriolinus @rphmeier @tomaka