Honeycomb integration proof-of-concept #5408
Conversation
| // Mute disables honeycomb entirely; useful in test environments. | ||
| Mute bool |
There was a problem hiding this comment.
Why "mute" rather than omitting the whole beeline config?
There was a problem hiding this comment.
Because it allows our test configs to be as close as possible to the real configs -- just a sed -i -e s/"mute": true/"writeKey": "deadbeef"/ away from being the real thing. I find that much less confusing than "why do our prod configs have this entire stanza that our test configs leave out?". It also leaves us room for things like having the beeline library log to stdout during tests if we want, eventually.
test/config/akamai-purger.json
Outdated
| "beeline": { | ||
| "mute": true, | ||
| "dataset": "Test", | ||
| "servicename": "AkamaiPurger" |
There was a problem hiding this comment.
nit: I'd kind of prefer servicename to be related in some formal way to some other string - for instance the binary name?
There was a problem hiding this comment.
Yeah that's a good idea. The closest thing Go has to this is os.Executable(), which is the full absolute path to the executable. Would you be happy with path.Base(os.Executable())?
There was a problem hiding this comment.
Sure! We do something similar in VersionString in cmd/shell.go:
func VersionString() string {
name := path.Base(os.Args[0])
return fmt.Sprintf("Versions: %s=(%s %s) Golang=(%s) BuildHost=(%s)", name, core.GetBuildID(), core.GetBuildTime(), runtime.Version(), core.GetBuildHost())
}
os.Executable() seems a little nicer than os.Args[0], so let's update both places.
There was a problem hiding this comment.
Ooh unfortunately os.Executable() can return an error, so it's not a great fit for VersionString(). But it works well here; lemme know what you think.
|
This generally looks good to me and I think is on the right track. I see you have marked this ready for review but also have an open TODO: "Fix "chained" interceptors to allow handler autodetection to work better." Do you want to resolve that in this PR, or file an issue for a subsequent PR? |
|
I'm very close to having it fixed, at least in a fork. I intend to resolve it in this PR, but the resolution won't have much impact on the meat of this PR, so let's start reviewing this as-is. |
jsha
left a comment
There was a problem hiding this comment.
Just a couple of tiny things. Overall this looks good. I filed a couple of upstream issues to see if we can trim off some of the dependencies.
| // WriteKey is the API key needed to send data Honeycomb. This can be given | ||
| // directly in the JSON config for local development, or as a path to a | ||
| // separate file for production deployment. |
There was a problem hiding this comment.
FYI, PasswordConfig is actually a transitional thing from 2016 we never cleaned up. 😳 We originally had passwords in the main JSON configs, then moved them out to separate files, in both prod and dev. But we never changed PasswordConfig to eliminate inline passwords. I think we should update this comment, and separately clean up PasswordConfig.
There was a problem hiding this comment.
For the sake of simplicity, I'd like to leave this as-is and then clean up PasswordConfig (and this usage of it) as a follow-up. #5426
|
(can't/won't land this until we understand what's going on with go-sql-driver 1.6.0, because this change depends on that update) |
|
I have resolved the go-sql-driver conflict using an |
Add Honeycomb tracing to all Boulder components which act as
HTTP servers, gRPC servers, or gRPC clients. Add many values
which we currently emit to logs to the trace spans. Add a way to
configure the Honeycomb integration to our config files, and by
default configure all of our tests to "mute" (send nothing).
Part of https://github.com/letsencrypt/dev-misc-tickets/issues/218