feat(rails): forward Rails.logger to PostHog Logs via OpenTelemetry#167
feat(rails): forward Rails.logger to PostHog Logs via OpenTelemetry#167johnnagro wants to merge 31 commits into
Conversation
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
posthog-rails/lib/posthog/rails/logs/setup.rb:136-140
The `service.version` OTel resource attribute is being set to `PostHog::VERSION`, which is the SDK/integration gem version — not the Rails application's version. Per OpenTelemetry semantic conventions, `service.version` represents the deployed application version, so this would cause every service to report the PostHog gem version as its "version" in PostHog Logs. Consider using `telemetry.sdk.version` for the SDK version instead, and omitting `service.version` from the defaults (users can supply it via `logs_resource_attributes`).
```suggestion
attrs = {
'service.name' => service_name,
'telemetry.sdk.name' => 'posthog-rails',
'telemetry.sdk.version' => PostHog::VERSION,
'deployment.environment' => ::Rails.env.to_s
}
```
### Issue 2 of 2
spec/posthog/rails/logs/appender_spec.rb:40-46
Non-parametric severity tests per project convention — `Severity::MAPPING` defines 6 entries (DEBUG→5, INFO→9, WARN→13, ERROR→17, FATAL→21, UNKNOWN→9) but only INFO and ERROR are verified. An RSpec `using` table (e.g., a `described_class` parametric approach with `let`/`subject` per row, or a shared example loop over `MAPPING`) would cover all cases with no duplication and catch a mapping regression for WARN, FATAL, or the UNKNOWN→INFO fallback.
Reviews (1): Last reviewed commit: "feat(rails): forward Rails.logger to Pos..." | Re-trigger Greptile |
|
Cc @PostHog/logs |
|
Thanks for taking the time to look this over and leave such thoughtful comments, I appreciate it @turnipdabeets @DanielVisca I made some changes based on your feedback. There is one open question about feature flags. |
| record = apply_before_send(record) | ||
| return if record.nil? | ||
|
|
||
| @otel_logger.on_emit(**record) |
There was a problem hiding this comment.
Can we include traceId/spanId? The other SDKs' log records carry the trace fields, and the OTel logs SDK's on_emit should resolve the current active span from ambient context (or an explicit context: argument) — so apps already running OTel tracing may get logs↔trace correlation for free here. Worth verifying against our gem versions: if it already works; if not, it's likely a one-line context: argument.
There was a problem hiding this comment.
Okay so, this "just works", I verified it against the source: on_emit defaults context: OpenTelemetry::Context.current and fills the top-level trace_id/span_id/trace_flags from the active span (https://github.com/open-telemetry/opentelemetry-ruby/blob/main/logs_sdk/lib/opentelemetry/sdk/logs/logger.rb). Apps that run the tracing instrumentation gems (opentelemetry-instrumentation-rails/-rack) get logs↔trace correlation for free; since our appender emits synchronously on the calling thread, the ambient context is the traced request's own.
My smoke test has no tracer, so the trace fields are empty as expected:
I did add a line to the README documenting that traced requests get trace_id/span_id on their log records automatically.
Thoughts?
There was a problem hiding this comment.
I think as it's document that the tracing instrumentation gems are needed it should be ok. Ideally down the road we'd populate trace context ourselves so no extra gems are needed.
|
PR is looking really good. We'll also need a .changeset file for release automation system to run |
|
thanks, @turnipdabeets - please take another look |
|
@johnnagro "Commits must have verified signatures" in order to merge - mind signing these? |
Add an opt-in integration that broadcasts Rails.logger output to PostHog Logs over OTLP, automatically correlated with the request's distinct_id and session_id captured by the existing request-context middleware. The OpenTelemetry gems (opentelemetry-sdk, opentelemetry-logs-sdk, opentelemetry-exporter-otlp-logs) are optional/soft dependencies loaded lazily at runtime, so the feature no-ops with a single warning when they are absent (and on Ruby < 3.3 where the logs SDK is unsupported). The logs token and host are derived from the configured PostHog client (config.api_key / config.host) with ENV fallbacks, so logs always target the same project as analytics. Exposes api_key/host readers on the core client to support this. Disabled by default; enable via PostHog::Rails.config.logs_enabled.
Co-authored-by: Anna Garcia <11654201+turnipdabeets@users.noreply.github.com>
…ds don't consume budget
…l.path) for log request attributes
…ad of silently dropping
…t starve the events flush
…gger for prefix consistency
…he Rails logger level
…icates and level= are unaffected; document silence/tagged gaps
… fails persistently
…art through the logs pipeline
dddc3a8 to
ce9ca0e
Compare
np - signed. |
…el-logs # Conflicts: # posthog-rails/README.md
dustinbyrne
left a comment
There was a problem hiding this comment.
Hey @johnnagro, thanks for this! I took a look and have a few comments, but otherwise this is looking pretty solid
| def remember_client_options(options) | ||
| return unless options.is_a?(Hash) | ||
|
|
||
| @client_api_key = options[:api_key] || options['api_key'] | ||
| @client_host = options[:host] || options['host'] | ||
| end |
There was a problem hiding this comment.
We should also consider test_mode here so that we don't emit logs in test
posthog-ruby/posthog-rails/lib/generators/posthog/templates/posthog.rb
Lines 111 to 112 in 0ab6692
| def body_for(message) | ||
| str = message.is_a?(String) ? message.dup : message.inspect | ||
| str = str.encode(Encoding::UTF_8, invalid: :replace, undef: :replace) unless str.encoding == Encoding::UTF_8 | ||
| str.valid_encoding? ? str : str.scrub | ||
| end |
There was a problem hiding this comment.
If logging an exception, e.g.:
Rails.logger.error(ex)It'll render as something like <#RuntimeError: unknown error occurred> instead of including the backtrace
we should consider doing something like Logger::Formatter#msg2str
| def install! | ||
| return @appender if @installed | ||
|
|
||
| @installed = true | ||
| return nil unless require_otel_gems |
There was a problem hiding this comment.
Worth checking here if the PostHog client is disabled and skipping installation if it is
We have this pattern in a lot of methods internal to the client:
posthog-ruby/lib/posthog/client.rb
Lines 293 to 294 in 0ab6692
posthog-ruby/lib/posthog/client.rb
Lines 208 to 209 in 0ab6692
@disabled isn't exposed, but I think it's fine to expose it as a public method. Maybe we opt for the positive case, though - client.enabled?
| # | ||
| # @param timeout [Numeric] Max seconds to spend; see {SHUTDOWN_TIMEOUT_SECONDS}. | ||
| # @return [void] | ||
| def shutdown!(timeout: SHUTDOWN_TIMEOUT_SECONDS) |
There was a problem hiding this comment.
nit considering this is private API space, but I think we should prefer a non-bang method name here (just shutdown)
it doesn't have a non-bang form, and it's not at risk of raising an exception
install! maybe has a stronger case given it's highly state changing, but I think we could do a non-bang there too
| gem 'oj', '~> 3.16.10' | ||
| # Soft dependency of posthog-rails' PostHog Logs feature; present here only | ||
| # so the fork-safety spec can exercise the real BatchLogRecordProcessor. | ||
| gem 'opentelemetry-logs-sdk', require: false |
There was a problem hiding this comment.
I tried running this as is, and got the following error
OpenTelemetry error: unexpected error in OTLP::Exporter#encode -
undefined method 'event_name' for an instance of OpenTelemetry::SDK::Logs::LogRecordData
My lockfile resolved to:
opentelemetry-logs-sdk 0.4.0
opentelemetry-exporter-otlp-logs 0.5.1
and it looks like the exporter expects log_record_data.event_name to exist, but opentelemetry-logs-sdk v0.4.0 defines LogRecordData without an event_name field.
a fresh bundle with the following fixed it:
gem 'opentelemetry-sdk', require: false # resolved to 0.6.0
gem 'opentelemetry-logs-sdk', require: false
gem 'opentelemetry-exporter-otlp-logs', require: false # resolved to 0.5.1maybe we use something like this?
| gem 'opentelemetry-logs-sdk', require: false | |
| gem 'opentelemetry-logs-sdk', '>= 0.6.0', require: false | |
| gem 'opentelemetry-exporter-otlp-logs', require: false |
There was a problem hiding this comment.
might be worth a test using real exports from these libs
💡 Motivation and Context
Adds an opt-in integration that forwards
Rails.loggeroutput to PostHog Logs(https://posthog.com/docs/logs) over OpenTelemetry/OTLP, automatically correlated
with the request's distinct_id and session_id captured by the existing
request-context middleware.
The OpenTelemetry gems are optional/soft dependencies loaded lazily, so the
feature no-ops with a single warning when absent (and on Ruby < 3.3 where the
logs SDK is unsupported). The logs token and host are derived from the configured
client (
config.api_key/config.host) with ENV fallbacks, so logs alwaystarget the same project as analytics. Disabled by default; enabled via
PostHog::Rails.config.logs_enabled.💚 How did you test it?
and railtie broadcast wiring (full suite green on Ruby 3.2.2, rubocop clean).
rails newapp against a real PostHog project:Rails.loggeroutput appears in PostHog Logs.📝 Checklist