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: add otel plumbing & grpc instrumentation #514

Merged
merged 17 commits into from
Sep 1, 2021

Conversation

tobert
Copy link
Contributor

@tobert tobert commented Jul 15, 2021

Description

This PR adds OpenTelemetry plumbing and gRPC instrumentation to tink. When configured as described in the README, this will cause tink components to emit otel tracing data to the configured service, which enables observability of requests across tink components.

Note: I couldn't seem to avoid updating some dependencies when I made these changes so there are some mostly-unrelated changes due to that, most notably, the Docker library and required API changes.

How Has This Been Tested?

Current tests seem to pass. Still need to add more tests which is why this is a draft PR for now.

How are existing users impacted? What migration steps/scripts do we need?

Currently the only way to trace requests across the tink stack is with logs and we'd like to improve on that.

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade
  • move to an otel-launcher that is supported (currently using @tobert's branch)

go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@tobert
Copy link
Contributor Author

tobert commented Aug 25, 2021

looks like sonatype-lift bot is over-sensitive to transitive checksums in go.sum

otel-init-go is using the newest version of OpenTelemetry and gRPC, newer than what was here before. Most of the things it's "detecting" are way out in the leaves of dependencies and as far as I can tell, do not actually get included in the binary produced.

Signed-off-by: Amy Tobey <atobey@equinix.com>
tstromberg
tstromberg previously approved these changes Aug 27, 2021
Seems to ditch some other old dependencies and make go vet happy.

Signed-off-by: Amy Tobey <atobey@equinix.com>
@tobert
Copy link
Contributor Author

tobert commented Aug 27, 2021

Fixing up those build fails, will need another review after @tstromberg :)

go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
@tobert
Copy link
Contributor Author

tobert commented Aug 27, 2021

While going through OSIE I realized I need to do some work in tink-worker and started that. I can tack that onto this PR or follow up. I should have something to look at in the middle of next week.

@tstromberg
Copy link
Contributor

While going through OSIE I realized I need to do some work in tink-worker and started that. I can tack that onto this PR or follow up. I should have something to look at in the middle of next week.

Let's put that into a follow-up PR.

@codecov
Copy link

codecov bot commented Aug 30, 2021

Codecov Report

Merging #514 (e94c7e9) into master (aaf6a8e) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #514      +/-   ##
==========================================
- Coverage   33.61%   33.59%   -0.02%     
==========================================
  Files          44       44              
  Lines        3385     3387       +2     
==========================================
  Hits         1138     1138              
- Misses       2150     2152       +2     
  Partials       97       97              
Impacted Files Coverage Δ
cmd/tink-worker/internal/action.go 0.00% <0.00%> (ø)
grpc-server/grpc_server.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aaf6a8e...e94c7e9. Read the comment docs.

@tstromberg
Copy link
Contributor

Can you run prettier on the README.md? It seems there is a github action that checks for it:

[warn] README.md
789
[warn] Code style issues found in the above file(s). Forgot to run Prettier?

(cue rant about linting markdown docs...)

Signed-off-by: Amy Tobey <atobey@equinix.com>
@tobert
Copy link
Contributor Author

tobert commented Aug 30, 2021

I've pushed up the prettier changes @tstromberg

@tstromberg
Copy link
Contributor

Looks like your PR is blocked on #529 (codespell doesn't like your go.mod file) - #530 may need to be merged first.

@tobert
Copy link
Contributor Author

tobert commented Sep 1, 2021

Wow, fun. This is totally breaking my branch. I'll figure it out.

@tstromberg tstromberg merged commit cbe2037 into tinkerbell:master Sep 1, 2021
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.

3 participants