Send telemetry logs for Profiling #3858
Conversation
TonyCTHsu
added
the
dev/internal
github-actions
bot
added
core
| "Failed to report profiling data (#{config_without_api_key}): " \ | ||
| "server returned unexpected HTTP #{result} status code" | ||
| ) | ||
| Datadog::Core::Telemetry::Logging.error("Failed to report profiling data: unexpected HTTP #{result} status code") | ||
| false | ||
| end | ||
| else | ||
| Datadog.logger.error("Failed to report profiling data (#{config_without_api_key}): #{result}") | ||
| Datadog::Core::Telemetry::Logging.error("Failed to report profiling data") | ||
| false | ||
| end |
There was a problem hiding this comment.
This seems like a lot of duplication: We're now reporting errors twice, to two different dd-trace-rb subsystems... And this will happen everywhere we have an error, and in the future a warning.
Would it be possible for Telemetry::Logging to also call the logger with the reported message?
This would mean that calls to Datadog.logger would be replaced with Telemetry::Logging.error, rather than copy-pasted.
There was a problem hiding this comment.
I am aware of the verbosity of those curated description. Currently it work around the constraints/risks for using telemetry logs:
- Must not transfer PII
- Reduce the cardinality in order to hash and batch
Combining the logger and telemetry logs in a single API is risky. Perhaps we can revisit the design in the future with more patterns and concrete examples.
There was a problem hiding this comment.
Ah. I wasn't aware of the requirement to reduce cardinality. I see how omitting details in these strings will have that effect.
Is this perhaps worth documenting in the telemetry logging class? E.g. So that if/when this question comes up again, other members of the team will have somewhere to reference.
There was a problem hiding this comment.
Will do that in the followup PR.
BenchmarksBenchmark execution time: 2024-08-21 14:37:33 Comparing candidate commit 0501d6d in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 22 metrics, 2 unstable metrics. scenario:profiler - Major GC runs (profiling enabled)
|
| "Failed to report profiling data (#{config_without_api_key}): " \ | ||
| "server returned unexpected HTTP #{result} status code" | ||
| ) | ||
| Datadog::Core::Telemetry::Logging.error("Failed to report profiling data: unexpected HTTP #{result} status code") | ||
| false | ||
| end | ||
| else | ||
| Datadog.logger.error("Failed to report profiling data (#{config_without_api_key}): #{result}") | ||
| Datadog::Core::Telemetry::Logging.error("Failed to report profiling data") | ||
| false | ||
| end |
There was a problem hiding this comment.
Ah. I wasn't aware of the requirement to reduce cardinality. I see how omitting details in these strings will have that effect.
Is this perhaps worth documenting in the telemetry logging class? E.g. So that if/when this question comes up again, other members of the team will have somewhere to reference.
What does this PR do?
Report internal error via telemetry logs for Profiling