Initial MBT plugin draft #236
Conversation
duncanpo
left a comment
duncanpo
left a comment
There was a problem hiding this comment.
Looks good. I left a few comments. Please address as appropriate
| "internal", ... | ||
| "proto", ... | ||
| ] ... | ||
| ); |
There was a problem hiding this comment.
These attributes are generated by OTel by default. You don't need to manually specify them here.
There was a problem hiding this comment.
After removing this code, I'm no longer seeing those attributes attached to the generated spans. Should I still keep the attribute assignments in that case?
There was a problem hiding this comment.
No, you don't need to keep them. Span kind is a part of the span although it is not an attribute. There is no need to have another attribute span.kind. otel.library.name I believe is an attempt capture the tracer name. Again that is automatically captured when starting a span (and it is also not an attribute). Regarding internal.span.format, I am currently having trouble locating it. But it certainly doesn't communicate anything important for the user, so I would leave that out too
| function runBuild(plugin, pluginData) | ||
| % Configure by attaching to span if passed in via environment | ||
| % variable, and propagating baggage | ||
| configureOTel(); |
There was a problem hiding this comment.
Looks like this will initialize/configure OTel at the start of each build and then clean up everything after. Process Advisor only initializes before the first build of a MATLAB session. If you run multiple builds within a MATLAB session, OTel would only be initialized once. Do you want to do something like that, which is more efficient?
There was a problem hiding this comment.
Hmm, that's an interesting question. Only setting up once is nice for improving performance, but if I understand correctly it allows for mutation of the environment in between runs right?
In order to ensure maximum reproducibility and determinism, having a consistent execution path through builds is quite powerful.
Overall, I think unless the performance costs prove to be expensive, I like reconfiguring each build to try and make the build process more hermetic. What are your thoughts there @duncanpo? Are there downsides to reconfiguration outside of the performance costs?
There was a problem hiding this comment.
I think it is reasonable to configure/clean up every build if performance cost doesn't prove prohibitive. But I would recommend providing a way to skip config/clean up (such as using an environment variable). That way if a customer already sets up OTel for something else, there would be at least a way to opt out of build tool overriding their config
| % baggage propagation | ||
| otelbaggage = getenv("BAGGAGE"); | ||
| if ~isempty(otelbaggage) | ||
| otelbaggage = split(split(string(otelbaggage),','), "="); |
There was a problem hiding this comment.
I know you took this code from Process Advisor. But as I am reading it, I realize it doesn't work if there is only one parameter in the baggage. Please add a reshape and it will work in all cases:
otelbaggage = reshape(split(split(string(otelbaggage),','), "="), [], 2);
There was a problem hiding this comment.
Good catch, I'll update that.
…ders with the MBT
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
This PR adds an MBT plugin + testing to make it for easy for users to instrument their MATLAB build tool builds with OpenTelemetry.
The plugin adds a trace for the build process, and creates spans for each task. In addition, it attaches information about the task context and build environment to these spans as attributes.
Finally, metrics are also added so that users can track quantitative data produced by their builds over time.