✨(summary) add hostname to analytics properties

[FloChehab]

This helps track down what was the source of events. This can be useful when checking perf of different workers for instance.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Add hostname to summary analytics event properties

✨ Enhancement 📝 Documentation 🕐 10-20 Minutes

AI Description

• Attach hostname to analytics events to identify the emitting worker.
• Preserve caller-provided hostname while defaulting when absent.
• Document the change in the changelog.

Diagram

graph TD
  A(["Summary worker/process"]) --> B["Analytics.capture()"] --> C{{"Analytics backend"}}
  B --> D["Add hostname property"] --> C

  subgraph Legend
    direction LR
    _svc(["Service/Process"]) ~~~ _mod["Module/Function"] ~~~ _ext{{"External system"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Set hostname once in analytics client context

• ➕ Avoids re-checking/injecting hostname on every capture call
• ➕ Centralizes global event metadata in one place
• ➖ May be harder to override per-event if the client merges properties differently
• ➖ Requires confidence in the underlying client’s global context behavior

2. Prefer explicit instance identity via env var (e.g., WORKER_ID/HOSTNAME)

• ➕ More stable/intentional identity across container restarts and orchestration setups
• ➕ Allows overriding hostname when network/DNS names are not meaningful
• ➖ Requires deployment configuration changes and documentation
• ➖ Still needs a fallback when not set

Recommendation: The current approach is a good default: it’s localized, low-risk, and preserves an explicitly provided hostname. If hostname becomes a required dimension for all events, consider moving it to a client-level context and/or supporting an explicit WORKER_ID env override for more meaningful instance identity in containerized deployments.

Files changed (2) +7 / -0

Enhancement (1) +6 / -0

analytics.pyInject hostname into analytics properties when missing +6/-0

Inject hostname into analytics properties when missing

• Imports socket and enriches event properties in capture() by defaulting properties["hostname"] to socket.gethostname() when not already provided. This helps attribute events to the worker/process emitting them.

src/summary/summary/core/analytics.py

Documentation (1) +1 / -0

CHANGELOG.mdDocument hostname enrichment for summary analytics +1/-0

Document hostname enrichment for summary analytics

• Adds a changelog entry noting that summary analytics events now include hostname in their properties.

CHANGELOG.md

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

1. Hostname errors escape wrapper 🐞 Bug ☼ Reliability

Description

Analytics.capture() now executes properties.get() and socket.gethostname() before the
try/except that wraps failures into AnalyticsException, so those errors can propagate as
arbitrary exceptions. MetadataManager.capture() only catches AnalyticsException, so an
unexpected exception here can bubble into Celery task/request execution paths.

Code

src/summary/summary/core/analytics.py[R47-50]

+        # We add hostname to help track down the source of events
+        properties = properties or {}
+        if not properties.get("hostname"):
+            properties["hostname"] = socket.gethostname()

Evidence

The hostname enrichment happens before the try: guarding _client.capture, so exceptions there
are not converted to AnalyticsException. Downstream, MetadataManager.capture() only catches
AnalyticsException, and Celery tasks invoke it directly; therefore, non-AnalyticsException
errors can propagate out of analytics and affect task execution.

src/summary/summary/core/analytics.py[42-58]
src/summary/summary/core/analytics.py[211-232]
src/summary/summary/core/celery_worker.py[615-620]
src/summary/summary/core/celery_worker.py[651-659]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Analytics.capture()` enriches `properties` with `hostname` *before* the existing `try/except` that wraps errors into `AnalyticsException`. Any exception from `properties` access (non-mapping, unusual mapping) or `socket.gethostname()` will bypass the wrapper and propagate upstream.

### Issue Context
`MetadataManager.capture()` only catches `AnalyticsException`, and Celery tasks call `metadata_manager.capture(...)` at the end of task execution. Keeping all enrichment inside the same wrapped boundary (or explicitly swallowing enrichment errors) preserves the previous contract that analytics failures should not crash task flows.

### Fix Focus Areas
- src/summary/summary/core/analytics.py[42-58]
- src/summary/summary/core/analytics.py[211-232]
- src/summary/summary/core/celery_worker.py[615-620]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Mutates caller properties dict 🐞 Bug ⚙ Maintainability

Description

Analytics.capture() adds hostname directly into the caller-provided properties mapping (when
it is truthy/non-empty and lacks hostname), creating a new side effect that can leak into
subsequent uses if the caller reuses the same dict. This is particularly relevant when callers pass
non-empty dicts (e.g., $set payloads).

Code

src/summary/summary/core/analytics.py[R48-50]

+        properties = properties or {}
+        if not properties.get("hostname"):
+            properties["hostname"] = socket.gethostname()

Evidence

The new code writes properties["hostname"] = ... directly to the properties object (when truthy
and missing hostname). Callers pass a dict (e.g., with $set), so that object is mutated as a
side effect of capture().

src/summary/summary/core/analytics.py[42-55]
src/summary/summary/api/route/tasks_v2.py[62-72]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Analytics.capture()` mutates the caller-provided `properties` dict in-place by adding `hostname`. This creates an unexpected side effect for callers that reuse the same mapping instance across calls.

### Issue Context
Some call sites construct a non-empty `properties` dict (e.g., adding `$set`), pass it to `analytics.capture(...)`, and may reasonably expect it to remain unchanged after the call.

### Fix Focus Areas
- src/summary/summary/core/analytics.py[42-55]
- src/summary/summary/api/route/tasks_v2.py[62-72]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
src/summary/summary/core/analytics.py	Adds a cached hostname property and merges it into each enabled analytics event.
CHANGELOG.md	Records the new summary analytics hostname property.

_{Reviews (3): Last reviewed commit: "fixup! ✨(summary) add hostname to analyt..." | Re-trigger Greptile}

[greptile-apps]

Hostname lookup escapes error boundary

If the operating system hostname lookup raises, socket.gethostname() executes before the analytics exception handler, causing task-submission requests to fail after enqueueing work and Celery failure handlers to stop before scheduling the client webhook.

[FloChehab]

I think it's ok

[lebaudantoine]

A few remarks:

1. I’m not sure we should allow the hostname to be overridden externally. With if not properties.get("hostname"), two different sources can populate the same field, and from PostHog it becomes unclear which one actually produced the value. Having a single way to set it would make the data much easier to reason about and analyze.

2. Why not resolve the value once at initialization? It doesn’t change between two capture() calls, so there’s no need to make the call for every event.

3. We’re currently using the low-level socket API here. I think we can get this information through an abstraction closer to Celery:

   • At a minimum, from celery.utils.nodenames import gethostname, which is Celery’s own (already memoized) wrapper.
   • Even better, use the worker’s node name (celery@host, or transcribe@%h if we name workers by queue). This is what Flower and inspect active display, so it would correlate nicely with the rest of our observability. We can retrieve it from the celeryd_init signal, where sender is the node name. That also addresses point 2: the value is resolved once when the worker starts, before the fork.

(We do have Flower deployed in grafana)

[FloChehab]

So I thought about it.

First of all, this helper is used outside of celery, so I wouldn't stick too much to it.
Secondly, the default celery node name is still celery@ so it would still be coherent in terms of infra (in our case / docker case).
Tbh, I was hoping posthog would automatically include the host name in its events.
If we wanted more fined grained analytics on celery related stuff I think we should make it explicit.

I have tweaked things regarding 1 & caching.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud