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(sat): telemetry support and startup / shutdown cleanup #56

Merged
merged 32 commits into from
Mar 25, 2022

Conversation

javorszky
Copy link
Contributor

@javorszky javorszky commented Mar 21, 2022

Closes #54

  • Reworked the startup / shutdown so there's a clearer place where everything happens rather than being distributed into different parts of the codebase.
  • Added tracing support that's configurable much the same way as Atmo is
  • Reworked tests to support the new functionality
  • Added a new Options to be behind Config for things that are statically set rather than dynamically figured out

Copy link
Contributor

@cohix cohix left a comment

Choose a reason for hiding this comment

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

Added a few comments. One thing I'm not seeing is the tracer being used anywhere. I would have expected it to be mounted on Sat's Vektor router and also some manual tracing being added to handleFnResult in meshed.go. Is that coming in another PR?

main.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
sat/sat.go Outdated Show resolved Hide resolved
@javorszky
Copy link
Contributor Author

@cohix I've added tracing code to both the vektor end of things, and the grav end of things.

The code assumes that the handler func in sat/handler.go file is the first and only entry point for http messages (code / routing seems to suggest that)

And it also assumes that the handleFnResult func in sat/meshed.go files it the first and only entry point for messages arriving via grav, and that the two send* funcs are only called from the handleFnResult func.

sat/handler.go Show resolved Hide resolved
sat/meshed.go Show resolved Hide resolved
sat/meshed.go Show resolved Hide resolved
sat/meshed.go Show resolved Hide resolved
sat/options/options.go Show resolved Hide resolved
sat/process/info.go Show resolved Hide resolved
Copy link
Contributor

@cohix cohix left a comment

Choose a reason for hiding this comment

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

After 58 is fixed up and merged, this is good to go! Well done.

* Add branching at startup for stdin / service modes

* Let's not do double work if not needed :D
@javorszky javorszky merged commit cdd2473 into main Mar 25, 2022
@javorszky javorszky deleted the gabor/54-otel-support branch March 25, 2022 15:51
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.

Add otel tracing support to sat
2 participants