feat: Adopt TraceExporter's Telemetry stuff - #183
Conversation
Overall package sizeSelf size: 30.38 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------|🤖 This report was automatically generated by heaviest-objects-in-the-universe |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa886f64ab
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
jules.wiriath@datadoghq.com unqueued this merge request |
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: waiting
mergequeue-status: removed
and removed
mergequeue-status: waiting
labels
This comment has been minimized.
This comment has been minimized.
| builder.set_otlp_headers(headers.clone()); | ||
| } | ||
| } | ||
| if let Some(cfg) = self.telemetry_config.take() { |
There was a problem hiding this comment.
Is it ok to take it instead of clone? Do we only use the config exactly once? It looks like the next time send_prepared_chunk will be called, telemetry_config will be gone.
There was a problem hiding this comment.
exporter_slot goes from None to Some, so the entire build block is skipped on every subsequent send_prepared_chunk (l529), if the build fails, build_error is latched and the next call returns early before reaching the take (l534), so yes it should be okay by line 563.
There was a problem hiding this comment.
Ok. I think I was confused by the fact that other values are cloned (headers, otlp endpoint) above instead of being taken out. Is that because we'll need them later?
There was a problem hiding this comment.
From a quick glance, build_error could be a Cell, but the others genuinely borrows from the content. If you think having all RefCell would help readability I don't mind too much, the impact of the borrow counting is probably minimal, otherwise I can make build_error also a Cell
There was a problem hiding this comment.
but the others genuinely borrows from the content.
But then we clone them, right? self.otlp_protocol.get() clones, and otlp_headers is also cloned L560. So I'm just wondering which possibility is it:
-
We use all those values
otlp_protocol,otlp_headers,telemetry_configexactly only once for the builder. In this case it's strange that they have a different treatment thantelemetry_config. We should rather useself.otlp_protocol.take()andstd::mem::take(self.otlp_headers.borrow_mut()). Maybe it doesn't really matter forOtlpProtcolwhich is stack-allocated anyway, but the headers are a vec, so it does avoid a clone. And in that situation in fact theotlp_headersshould probably not even be aRefCell, but aCell? Unless it's built in several steps? -
Some of these values are re-used elsewhere afterwards, so we need to let them live in
self. Then the current version makes sense, and this was basically my question aboveIs that because we'll need them later?
It's just not obvious that e.g.
headersandtelemetry_configshould be treated differently
There was a problem hiding this comment.
Yeah you are right it's situation 1, otlp_protocol is Copy and otlp_headers is cloned, they should also be Cells for I don't think they are reused elsewhere
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
This merge request conflicts with another merge request ahead in the queue. The merge requests in front of this one are: |
3 participants
No description provided.