Skip to content

Conversation

@jerbly
Copy link
Contributor

@jerbly jerbly commented Dec 1, 2025

Whenever a PolicyFinding is created in live-check a log_record is emitted to your configured OTLP endpoint. Here's an example from a Collector in debug mode:

Flags: 0
LogRecord #21
ObservedTimestamp: 2025-12-01 00:04:58.49238 +0000 UTC
Timestamp: 1970-01-01 00:00:00 +0000 UTC
SeverityText: INFO
SeverityNumber: Info(9)
EventName: weaver.live_check.finding
Body: Str(Conditionally required attribute 'server.port' is not present.)
Attributes:
     -> weaver.finding.id: Str(conditionally_required_attribute_not_present)
     -> weaver.finding.level: Str(information)
     -> weaver.finding.context.attribute_name: Str(server.port)
     -> weaver.sample.type: Str(histogram_data_point)
     -> weaver.sample.signal_type: Str(metric)
     -> weaver.sample.signal_name: Str(rpc.client.duration)
Trace ID: 
Span ID: 

...and a screenshot from a back-end:
image

@codecov
Copy link

codecov bot commented Dec 1, 2025

Codecov Report

❌ Patch coverage is 86.52344% with 69 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.3%. Comparing base (33083ad) to head (1b10784).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
...rates/weaver_live_check/src/advice/type_advisor.rs 77.2% 36 Missing ⚠️
...weaver_live_check/src/advice/deprecated_advisor.rs 80.3% 11 Missing ⚠️
...rates/weaver_live_check/src/advice/rego_advisor.rs 83.6% 8 Missing ⚠️
crates/weaver_live_check/src/lib.rs 64.2% 5 Missing ⚠️
crates/weaver_live_check/src/otlp_logger.rs 94.1% 5 Missing ⚠️
...rates/weaver_live_check/src/advice/enum_advisor.rs 94.2% 2 Missing ⚠️
crates/weaver_live_check/src/advice/mod.rs 93.7% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1045     +/-   ##
=======================================
+ Coverage   79.0%   79.3%   +0.3%     
=======================================
  Files         85      92      +7     
  Lines       7034    7331    +297     
=======================================
+ Hits        5558    5817    +259     
- Misses      1476    1514     +38     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@martinjt
Copy link
Member

martinjt commented Dec 1, 2025

Could the message follow a message template over using interpolated strings?

So something like Conditionally required attribute {weaver.finding.context.attribute_name} is not present?

@jsuereth
Copy link
Contributor

jsuereth commented Dec 1, 2025

Very cool!

@jerbly
Copy link
Contributor Author

jerbly commented Dec 1, 2025

Could the message follow a message template over using interpolated strings?

So something like Conditionally required attribute {weaver.finding.context.attribute_name} is not present?

@martinjt - not super easily, wondering what the use case is?

@martinjt
Copy link
Member

martinjt commented Dec 2, 2025

The idea is that body should be low cardinality, that's always the way I've seen it. I kinda thought that was how all logging frameworks worked? Maybe thats just a go and .net thing though?

Admittedly, events and event ID should solve that usecase. The idea being that you can group together logs that are the same, but have different attributes when querying and aggregating the data.

@jerbly
Copy link
Contributor Author

jerbly commented Dec 2, 2025

Oh, I see, I did not know that was the case for body - I thought that was where the human readable text went. I guess I could move it to a weaver.finding.message attribute?

@martinjt
Copy link
Member

martinjt commented Dec 2, 2025

The way it works in .net is that there is a FormattedMessage attribute that you can opt in to having populated.

@jerbly
Copy link
Contributor Author

jerbly commented Dec 2, 2025

I'm wondering if this is a specific thing that dot net does? The docs in the proto file, for example, say this:

  // A value containing the body of the log record. Can be for example a human-readable
  // string message (including multi-line) describing the event in a free form or it can
  // be a structured data composed of arrays and maps of other values. [Optional].
  opentelemetry.proto.common.v1.AnyValue body = 5;

I have not used logs much myself so looking for more opinions here before going one way or the other: @open-telemetry/weaver-approvers

@jsuereth
Copy link
Contributor

jsuereth commented Dec 2, 2025

I'm wondering if this is a specific thing that dot net does? The docs in the proto file, for example, say this:

  // A value containing the body of the log record. Can be for example a human-readable
  // string message (including multi-line) describing the event in a free form or it can
  // be a structured data composed of arrays and maps of other values. [Optional].
  opentelemetry.proto.common.v1.AnyValue body = 5;

I have not used logs much myself so looking for more opinions here before going one way or the other: @open-telemetry/weaver-approvers

I don't believe Java does this, not sure for other languages, but I don't think it's the "norm" or at least, it's not specified logging should do this.

@martinjt
Copy link
Member

martinjt commented Dec 2, 2025

I think it depends on the framework you're doing for sure. There's a list of the frameworks supporting it at the bottom of the page.

https://messagetemplates.org/

@jerbly jerbly changed the title [WIP] Policy Findings emitted as OTLP log records as they arise Policy Findings emitted as OTLP log records as they arise Dec 3, 2025
@jerbly jerbly marked this pull request as ready for review December 3, 2025 15:56
@jerbly jerbly requested a review from a team as a code owner December 3, 2025 15:56
use weaver_checker::{FindingLevel, PolicyFinding};
use weaver_semconv::deprecated::Deprecated;

use super::{Advisor, FindingBuilder};
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious what's the difference between advise and finding. Could we unify them and use one term to describe both? This applies to APIs and attributes/event naming

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Until very recently we had:

  • Advisors who advise and produce Advice

Now we have:

  • Advisors who advise and produce PolicyFindings

I'm not sure what I would rename Advisor and the verb advise to?

Copy link
Member

Choose a reason for hiding this comment

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

Could Advisors produce PolicyAdvices ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have Advisors produce Findings. The Findings can be advice violation or warning.

Previously Advice as a violation was a bit awkward. So I don't think we're any "worse" here if Advisors produce a Finding - where finding could be one of these three things.


/// Enable OTLP log emission for live check policy findings
#[arg(long, default_value = "false")]
emit_otlp_logs: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Should we bundle these into a struct?

@jerbly jerbly enabled auto-merge (squash) December 5, 2025 02:44
@jerbly jerbly merged commit 5f71818 into open-telemetry:main Dec 5, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants