feat(otel): advanced observability phase 2 - data layer and privacy hardening by jaegeral · Pull Request #3797 · google/timesketch

jaegeral · 2026-05-05T13:59:00Z

This Pull Request implements Phase 2 of the OpenTelemetry instrumentation for Timesketch. The focus of this phase is extending distributed tracing into the data layer (OpenSearch and PostgreSQL)

Data Layer Visibility

OpenSearch: Implemented manual instrumentation for search() and count() methods. This captures targeted indices and internal processing latency (took_ms) without exposing the query content.
SQLAlchemy (PostgreSQL): Integrated automatic tracing for all database operations, providing visibility into connection health and statement execution time.
ACL Layer: Wrapped has_permission() checks in logical spans. This groups the many individual database metadata lookups (often 40+ per request) into a single, understandable context in the trace waterfall.
Content Exclusion: Raw search query strings and DSL structures are now completely excluded from span attributes. This ensures that no sensitive case data (PII, hostnames, identifiers) ever leaves the application via telemetry.
Sensitive Data Scrubber: A global SpanProcessor automatically redacts credentials (passwords, tokens, sessions) from any span attribute across the entire system.
Analyst Attribution: Authenticated user.id and user.name are captured on all API spans. This allows correlating performance bottlenecks or errors back to a specific investigator without seeing their search terms.-

Infrastructure & Documentation

Configuration: Relocated otel-collector-config.yaml to the central data/ directory for better deployment readiness.
Guides: Updated docs/OpenTelemetry.md with current architecture and configuration details.

jaegeral · 2026-05-05T13:59:13Z

/gemini review

jaegeral · 2026-05-06T07:21:51Z

      - CHOKIDAR_USEPOLLING=true
      - PROMETHEUS_MULTIPROC_DIR=/tmp/
-      - ENABLE_STRUCTURED_LOGGING=true
+      # - ENABLE_STRUCTURED_LOGGING=true


#3795 (comment)

jaegeral · 2026-05-06T07:22:09Z

#3795 (comment)

jaegeral · 2026-05-06T07:57:34Z

Had timesketch/models/acl.py:324:4: W9016: "permission, user" missing in parameter type documentation (missing-type-doc)

jkppr

General:
Your PR says Safe Telemetry Calls: Implemented a @safe_telemetry_call decorator ... but the whole diff does not contain this decorator at all. Can you please ensure the PR message reflects what the implementation actually contains?

jkppr · 2026-05-08T07:14:03Z

Are there any opentelemetry modules we can enable on our Opensearch cluster to get better insights from the other end too?

jkppr · 2026-05-08T07:28:36Z

+
+                # 2. Check for sensitive keywords in the value
+                lower_val = value.lower()
+                if any(keyword in lower_val for keyword in SENSITIVE_KEYWORDS):


Since the SENSITIVE_KEYWORDS contain pretty general stuff like key or auth, this will scrub data a bit too eagerly don't you think? e.g., searching for "windows_auth" or an error saying "index key_logs missing" will be redacted

Maybe limit value-based redaction to regex word boundaries or only run value-checks against suspicious keys rather than unconditionally across all string attributes. WDYT?

let me see, I had a better idea yesterday evening on how to avoid any search related things to spill.

jkppr · 2026-05-08T07:31:01Z

+    Returns:
+        opentelemetry.trace.Tracer: A tracer instance.
+    """
+    return trace.get_tracer(name)


If I'm not mistaken this will lead to application crashes if HAS_OTEL=False. Either we need to return a dump tracker or ensure with other methods that the with blocks in the core application (e.g. ACL.py) don't crash.

jkppr · 2026-05-08T07:32:55Z

+                if is_credential_key:
+                    redacted_keys.append(key)
+                    # pylint: disable=protected-access
+                    span._attributes[key] = "[REDACTED]"


Why do we need to mutate a protected member here?

jkppr · 2026-05-08T07:39:53Z

-            permission = codecs.decode(permission, "utf-8")
-        return self._get_ace(permission=permission, user=user)
+        tracer = telemetry.get_tracer(__name__)
+        with tracer.start_as_current_span("acl.has_permission") as span:


If what your PR message says is true (one request can generate 40+ calls), doesn't this wrapper now create 40+ tiny spans per request which will result in a bloat of traces and load on the GCP or Jaeger end?
Do we need this level of granularity or could we also aggregate AACL checks into a single parent span?

Also: How does this impact memory usage of the Timesketch services? I imagine the trace tree per request with multiple users is rather large?

yes it would create that many, but that is also true if we just enable sqlalchemy tracing :-/

jkppr · 2026-05-08T07:44:18Z

+        self.assertEqual(attrs["custom_field"], "[REDACTED]")
+
+        # Verify analyst identity is NOT redacted
+        self.assertEqual(attrs["user.name"], "analyst@google.com")


Wait, this is not redacted because it does not contain any of the keywords used on your list. So wouldn't this test be more accurate when you check for auth-analyst@google.com instead?

And we should not use @google.com here. Maybe use @example.com instead.

jkppr · 2026-05-08T07:55:37Z

+    # Attributes that are explicitly exempt from PII redaction (e.g. analyst identity)
+    EXEMPT_PII_ATTRIBUTES = {"user.name", "user.id", "timesketch.user_id"}
+
+    class SensitiveDataScrubber(SpanProcessor):


How does this impact performance? If this scrubs every string doing a sub-string search for a list of keywords to redact, won't this impact the API and worker performance of Timesketch?

Would it be sufficient to work only on a list of known risk attribute keys?

jaegeral · 2026-05-08T12:34:02Z

I think I will need to split up this PR. To messy to review

jaegeral · 2026-05-12T07:59:50Z

/gemini review

gemini-code-assist

Code Review

This pull request significantly expands OpenTelemetry integration by adding manual tracing for OpenSearch queries and ACL checks, implementing SQLAlchemy auto-instrumentation, and introducing a PII scrubber for spans. It also improves log correlation by injecting trace context into application logs. The code review identified several critical issues where the application could crash if OpenTelemetry dependencies are missing, specifically due to unhandled exceptions in the logging and tracer initialization paths. Additionally, feedback highlighted that the PII scrubbing logic is overly aggressive and fails to use the defined exemption list, and noted a discrepancy in the documentation regarding search query capturing.

gemini-code-assist · 2026-05-12T08:01:35Z

 except ImportError:
    HAS_OTEL = False


Since TraceLogFilter is defined inside the try block, it will not exist if the opentelemetry package is missing. This will cause an AttributeError in timesketch/app.py at line 286, crashing the application. You should provide a fallback implementation of TraceLogFilter in this except block.

Suggested change

except ImportError:

HAS_OTEL = False

except ImportError:

HAS_OTEL = False

class TraceLogFilter(logging.Filter):

"""Fallback filter when OpenTelemetry is not installed."""

def filter(self, record):

record.trace_id = "0" * 32

record.span_id = "0" * 16

return True

gemini-code-assist · 2026-05-12T08:01:35Z

+    Returns:
+        opentelemetry.trace.Tracer: A tracer instance.
+    """
+    return trace.get_tracer(name)


This function will raise a NameError if the opentelemetry package is not installed, as the trace module is only imported within the try block at the top of the file. Since get_tracer is now called in core modules like opensearch.py and acl.py, this will crash the application for any user without the optional telemetry dependencies. This function must check HAS_OTEL and return a no-op tracer if it is False.

gemini-code-assist · 2026-05-12T08:01:35Z

+
+                # 2. Check for sensitive keywords in the value
+                lower_val = value.lower()
+                if any(keyword in lower_val for keyword in SENSITIVE_KEYWORDS):


Checking for keywords like 'key' or 'session' within all attribute values is extremely aggressive for a digital forensics tool. This will likely cause significant data loss in telemetry; for example, any SQL statement containing a 'sessions' table or any registry path containing the word 'key' will be completely redacted. Consider removing these common words from the value-based check or using more precise matching (e.g., regex for tokens/passwords).

gemini-code-assist · 2026-05-12T08:01:35Z

+            if not HAS_OTEL:
+                return True


If HAS_OTEL is False, this filter returns early without setting record.trace_id or record.span_id. This will cause a KeyError in timesketch/app.py at line 308 because the log formatter expects these attributes to exist. You should always ensure these attributes are initialized with default values (e.g., zeros) before checking HAS_OTEL.

Suggested change

if not HAS_OTEL:

return True

def filter(self, record):

record.trace_id = "0" * 32

record.span_id = "0" * 16

if not HAS_OTEL:

return True

gemini-code-assist · 2026-05-12T08:01:35Z

-*   **Async Exporting:** Spans are exported asynchronously using a `BatchSpanProcessor` to ensure minimal impact on application performance.
+*   **Flask:** Captures all HTTP requests, status codes, and analyst identity.
+*   **Celery:** Maintains trace context across background tasks (analyzers, data imports).
+*   **OpenSearch:** Manual instrumentation captures search query structure (`db.statement`), targeted indices, and internal processing time (`took_ms`).


This line states that search query structure is captured in db.statement, but the PR description and the implementation in timesketch/lib/datastores/opensearch.py indicate that query content is explicitly excluded for privacy hardening. Please update this documentation to reflect the actual behavior.

gemini-code-assist · 2026-05-12T08:01:35Z

+
+            redacted_keys = []
+            # Create a copy of keys to avoid modification during iteration
+            for key in list(span.attributes.keys()):


The EXEMPT_PII_ATTRIBUTES set (defined at line 71) is never used in the on_end method. This means analyst identity attributes like user.name or user.id could be accidentally redacted if they happen to contain one of the sensitive keywords, which contradicts the goal of maintaining analyst attribution.

Suggested change

for key in list(span.attributes.keys()):

for key in list(span.attributes.keys()):

if key in EXEMPT_PII_ATTRIBUTES:

continue

# Protect our own audit info

Telemetry Privacy & Redaction and add a span for acl checks

5e759f4

jaegeral added the Observability label May 5, 2026

jaegeral added 3 commits May 5, 2026 14:02

black formatter

d912202

pylint

16841e9

doc update

6ab697a

jaegeral requested a review from rgayon May 5, 2026 14:07

jaegeral self-assigned this May 5, 2026

jaegeral marked this pull request as ready for review May 5, 2026 14:07

jaegeral and others added 8 commits May 5, 2026 14:24

romaion feedback

49ad0e8

lint

a87874c

Merge branch 'master' into otel-advanced-observability

909010e

# - ENABLE_STRUCTURED_LOGGING=true

d780af5

move otel-collector-config.yaml

21fb629

remove search query and sql query from traces

3eb9b47

lint

b59987e

lint and black

57a274d

jaegeral commented May 6, 2026

View reviewed changes

Comment thread data/otel-collector-config.yaml

Copy link
Copy Markdown

Collaborator Author

jaegeral May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3795 (comment)

jaegeral added 2 commits May 6, 2026 07:29

lint and black

9ee0b00

update doc

b676902

jaegeral commented May 6, 2026

View reviewed changes

Comment thread timesketch/lib/datastores/opensearch.py

lint and black

11c82a7

jaegeral changed the title ~~feat: Telemetry Privacy & Redaction and add a span for acl checks~~ feat(otel): advanced observability phase 2 - data layer and privacy hardening May 6, 2026

lint and black

899ccc3

jaegeral requested review from jkppr May 6, 2026 07:57

jaegeral mentioned this pull request May 6, 2026

feat(otel): instrument LLM providers for latency and prompt visibility #3798

Draft

jkppr requested changes May 8, 2026

View reviewed changes

jaegeral marked this pull request as draft May 12, 2026 06:22

gemini-code-assist Bot reviewed May 12, 2026

View reviewed changes

-            for key in list(span.attributes.keys()):
+            for key in list(span.attributes.keys()):
+                if key in EXEMPT_PII_ATTRIBUTES:
+                    continue
+                # Protect our own audit info

Conversation

jaegeral commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Data Layer Visibility

Infrastructure & Documentation

Uh oh!

jaegeral commented May 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaegeral commented May 6, 2026

Uh oh!

jkppr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaegeral commented May 8, 2026

Uh oh!

jaegeral commented May 12, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jaegeral commented May 5, 2026 •

edited

Loading