-
Notifications
You must be signed in to change notification settings - Fork 42
feat: instrument the snap charm library #164
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
Conversation
james-garner-canonical
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing unit and integration tests should make sure that we're not breaking anything here, but it might be a good idea to add something to the tests just to ensure that we can fetch the expected traces -- probably easiest to do that in the unit tests. The integration tests are not actually charm integration tests though, so if there's a lightweight way to check on the traces locally then the integration tests could be a better place.
|
Re: tests, I'm working on unit tests. |
james-garner-canonical
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can accept the argument (made elsewhere) that if the OTEL API is being called correctly, we don't need to further test it. The improved coverage of these functions that were previously mocked out is a nice improvement, and ensures we cover the API call sites. I think these tests are sufficient as is, if it's a lot of work to interact with OTEL more deeply in tests -- and as mentioned, probably not something we should test directly.
This PR instruments the
snapcharm lib:This roughly follows Ops instrumentation paradigm, where hook tool and Pebble calls are traced on the process boundary.
Ref #160