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

Add otel logs to rideshare #2476

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

simonswine
Copy link
Contributor

@simonswine simonswine commented Oct 2, 2023

In order to show better how the data signals connect I am adding a otel log pusher to our rideshare demo.

Note: The upgrade of otel required an upgrade of dskit, which needed some refactoring in commit f241af5.

Unfortunately the rideshare-app does not log at present, so I had to add a bit of logging as well

Screen.Recording.2023-10-03.at.16.04.21.mov

@simonswine simonswine force-pushed the 20231002_rideshare-otel-logs branch 4 times, most recently from 420ae52 to f241af5 Compare October 4, 2023 07:16
@simonswine simonswine marked this pull request as ready for review October 4, 2023 07:17
@simonswine simonswine requested review from korniltsev and a team as code owners October 4, 2023 07:17
Copy link
Collaborator

@korniltsev korniltsev left a comment

Choose a reason for hiding this comment

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

lgtm
Dont understand why updating otel in rideshare requires updates in separate modules? Cant different modules have different dependencies?
Maybe sometime later we should remove rideshare examples from the workspace

@korniltsev
Copy link
Collaborator

nit: may be it is worth considering cherry-pick dskit upgrade in a separate PR

@simonswine
Copy link
Contributor Author

simonswine commented Oct 4, 2023

Dont understand why updating otel in rideshare requires updates in separate modules? Cant different modules have different dependencies? Maybe sometime later we should remove rideshare examples from the workspace

No idea why it is part of the workspace, maybe the go push library was part of this repo at some point. But the fact it is in the same workspace forces the same grpc version, that then requires dskit upgrades. Once I am back at the keyboard I will split it or just remove it from the workspace.

@simonswine
Copy link
Contributor Author

Once I remove it from go.work, it seems to ignore the local go.mod file and can't build it anymore. This is very weird:

in the examples/golang-push/rideshare folder
$ go build
main module (github.com/grafana/pyroscope) does not contain package github.com/grafana/pyroscope/examples/golang-push/rideshare

$ go run main.go
main.go:16:2: no required module provides package github.com/agoda-com/opentelemetry-logs-go; to add it:
        go get github.com/agoda-com/opentelemetry-logs-go
main.go:17:2: no required module provides package github.com/pyroscope-io/otel-profiling-go; to add it:
        go get github.com/pyroscope-io/otel-profiling-go
main.go:9:2: package rideshare/bike is not in GOROOT (/nix/store/h2m71h2gy2kr2xvviccxa0mg5jayngcs-go-1.20.8/share/go/src/rideshare/bike)
main.go:10:2: package rideshare/car is not in GOROOT (/nix/store/h2m71h2gy2kr2xvviccxa0mg5jayngcs-go-1.20.8/share/go/src/rideshare/car)
main.go:11:2: package rideshare/rideshare is not in GOROOT (/nix/store/h2m71h2gy2kr2xvviccxa0mg5jayngcs-go-1.20.8/share/go/src/rideshare/rideshare)
main.go:12:2: package rideshare/scooter is not in GOROOT (/nix/store/h2m71h2gy2kr2xvviccxa0mg5jayngcs-go-1.20.8/share/go/src/rideshare/scooter)

@korniltsev
Copy link
Collaborator

maybe GOWORK=off can help building?

GOWORK=off go build  -o qwe

@simonswine
Copy link
Contributor Author

Yeah that works, unsure if we should keep it in the go.work file then?

@simonswine
Copy link
Contributor Author

Ok I think an "empty" go work file in the examples also does the trick 🙂

@simonswine simonswine merged commit 1e2ed25 into grafana:main Oct 4, 2023
16 checks passed
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.

None yet

2 participants