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(nodebuilder): Invoke traces from fx #2477

Merged
merged 5 commits into from
Jul 14, 2023

Conversation

vgonkivs
Copy link
Member

Overview

Invoke traces from the fx

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@vgonkivs vgonkivs added kind:feat Attached to feature PRs area:metrics Related to measuring/collecting node metrics labels Jul 14, 2023
@vgonkivs vgonkivs self-assigned this Jul 14, 2023
@vgonkivs vgonkivs requested a review from Bidon15 July 14, 2023 07:56
@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2023

Codecov Report

Merging #2477 (9d7dfb1) into main (05e00e8) will decrease coverage by 0.16%.
The diff coverage is 9.09%.

@@            Coverage Diff             @@
##             main    #2477      +/-   ##
==========================================
- Coverage   52.76%   52.61%   -0.16%     
==========================================
  Files         156      156              
  Lines        9966     9993      +27     
==========================================
- Hits         5259     5258       -1     
- Misses       4241     4273      +32     
+ Partials      466      462       -4     
Impacted Files Coverage Δ
nodebuilder/settings.go 45.94% <9.09%> (-18.34%) ⬇️

... and 7 files with indirect coverage changes

nodebuilder/settings.go Outdated Show resolved Hide resolved
nodebuilder/settings.go Outdated Show resolved Hide resolved
Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>
@vgonkivs
Copy link
Member Author

vgonkivs commented Jul 14, 2023

One more optimisation that we can do is to pass []*resource.Resource to initMetrics/initTraces as this options are equal in both metrics and traces:

resource.NewWithAttributes(
			semconv.SchemaURL,
			semconv.ServiceNamespaceKey.String(fmt.Sprintf("Celestia-%s", nodeType)),
			semconv.ServiceNameKey.String(fmt.Sprintf("semver-%s", buildInfo.SemanticVersion)),
			semconv.ServiceInstanceIDKey.String(fmt.Sprintf("%s/%s", network.String(), peerID.String()))),
		)

nodebuilder/settings.go Outdated Show resolved Hide resolved
nodebuilder/settings.go Outdated Show resolved Hide resolved
@renaynay
Copy link
Member

As I understand, it's still being tested by @jrmanes

@renaynay renaynay changed the title feat: invoke traces from fx feat(nodebuilder): Invoke traces from fx Jul 14, 2023
@tty47
Copy link
Contributor

tty47 commented Jul 14, 2023

As I understand, it's still being tested by @jrmanes

yes @renaynay , I'm in contact with Slava about this topic :)

nodebuilder/settings.go Outdated Show resolved Hide resolved
@tty47
Copy link
Contributor

tty47 commented Jul 14, 2023

hello team!

regarding the latest commit:

This is how it will look like:

We could find the network/peer.ID in the services:

Screenshot 2023-07-14 at 16 43 41

and the node type in the metrics:
Screenshot 2023-07-14 at 16 44 09

thank you so much @vgonkivs 😃

@Wondertan Wondertan enabled auto-merge (squash) July 14, 2023 18:43
@Wondertan Wondertan merged commit 9c40a7f into celestiaorg:main Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Related to measuring/collecting node metrics kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants