-
Notifications
You must be signed in to change notification settings - Fork 13
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: store tracer configuration in an in-memory file #184
base: main
Are you sure you want to change the base?
Conversation
Resolves [APMAPI-1066]
BenchmarksBenchmark execution time: 2025-02-11 13:24:28 Comparing candidate commit 6f71825 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #184 +/- ##
==========================================
- Coverage 93.80% 93.62% -0.18%
==========================================
Files 73 73
Lines 4195 4252 +57
==========================================
+ Hits 3935 3981 +46
- Misses 260 271 +11 ☔ View full report in Codecov by Sentry. |
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.
LGTM, because although there's a couple of questions, being aware of those concerns is enough
} | ||
|
||
Expected<InMemoryFile> InMemoryFile::make(StringView name) { | ||
int fd = memfd_create(name.data(), MFD_CLOEXEC | MFD_ALLOW_SEALING); |
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.
Question: The RFC suggest to use libdatadog for this. Is this temporary or will the cpp tracer now follow that advice?
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.
It suggest, doesn't enforce. Since dd-trace-cpp
has no dependency on libdatadog
, it will not be that efficient to use it.
@@ -110,6 +114,42 @@ std::string Tracer::config() const { | |||
return config.dump(); | |||
} | |||
|
|||
void Tracer::store_config() { |
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.
Can this lead to incorrect usage? If store_config is called twice, will that result in multiple descriptors for this process? If so, what are the implications of that?
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.
Every call to this method will result in N
file descriptors. Since Tracer::store_config()
is a private method and we control how it’s called, I believe we can ensure it’s used correctly and avoid any unintended issues.
Description
Resolves APMAPI-1066