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: instrument traceroute check #186

Merged
merged 16 commits into from
Oct 8, 2024
Merged

Conversation

lvlcn-t
Copy link
Member

@lvlcn-t lvlcn-t commented Sep 10, 2024

Motivation

Partly addresses #111 but could also be extended to the other checks and parts of the sparrow to provide technical information about the sparrow.

Changes

I've instrumented the traceroute check to provide more information about how the traceroute performed. The traceroute check now starts a span for each target to trace the route to. In this span each hop then starts a new span to provide information about the hop. The spans are then sent to the configured telemetry backend.

For additional information look at the commits.

Tests done

  • Unit tests succeeded - Not applicable for this PR
  • E2E tests succeeded

Manual e2e tests

Here is how the traces will then look like in Jaeger:

image

And this is how a trace then looks like:

image

TODO

  • I've assigned this PR to myself
  • I've labeled this PR correctly

@lvlcn-t lvlcn-t added feature Introduces a new feature area/checks Issues/PRs related to Checks area/metrics Issues/PRs related to metrics labels Sep 10, 2024
@lvlcn-t lvlcn-t self-assigned this Sep 10, 2024
Signed-off-by: Bruno Bressi <[email protected]>
Copy link
Member

@puffitos puffitos left a comment

Choose a reason for hiding this comment

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

Some questions regarding missing error spans mostly.

pkg/checks/traceroute/check.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Show resolved Hide resolved
* refactor: use checkname as trace name
* refactor: use explicit default case
* fix: set error statuses for errors
* refactor: function naming
* feat: add more span attributes
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
pkg/checks/traceroute/traceroute.go Outdated Show resolved Hide resolved
* chore: rm named returns
* chore: mark no route to host error as error
* chore: rename hop method to doHop

Signed-off-by: lvlcn-t <[email protected]>
@lvlcn-t lvlcn-t removed the request for review from eumel8 October 7, 2024 11:58
@lvlcn-t lvlcn-t merged commit 98726ae into main Oct 8, 2024
8 checks passed
@lvlcn-t lvlcn-t deleted the feat/traceroute-otel-metrics branch October 8, 2024 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/checks Issues/PRs related to Checks area/metrics Issues/PRs related to metrics feature Introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants