Skip to content

[Fix] Render datalayer.Model and ScoredModel in structured logs - #281

Open
davidbreitgand wants to merge 2 commits into
llm-d:mainfrom
davidbreitgand:fix/fix-model-log-rendering
Open

[Fix] Render datalayer.Model and ScoredModel in structured logs#281
davidbreitgand wants to merge 2 commits into
llm-d:mainfrom
davidbreitgand:fix/fix-model-log-rendering

Conversation

@davidbreitgand

Copy link
Copy Markdown
Contributor

/kind bug

What this PR does / why we need it:

Fixes rendering Model and ScoredModel in structured logs.

datalayer.Model and ScoredModel serialized to {} and {"Score":X} with the name dropped in DEBUG log lines because the
concrete types have only unexported / embedded-interface fields.

This PR adds
MarshalJSON on *model projecting name, and on *ScoredModel projecting
name + score so the promoted embedded method does not silently drop
scores.

Attributes are intentionally not projected because AttributeMap is itself an
interface backed by an unexported sync.Map and would recurse into the
same {} problem.

Fixes #280

NONE

…red logs

Signed-off-by: David Breitgand <davidbreitgand@users.noreply.github.com>
@github-actions github-actions Bot added kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 5, 2026
@davidbreitgand

Copy link
Copy Markdown
Contributor Author

/cc @nirrozenbaum

@github-actions
github-actions Bot requested a review from nirrozenbaum August 5, 2026 14:01

@nirrozenbaum nirrozenbaum left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks @davidbreitgand,
what are your thoughts about implementing String() instead of MarshalJSON?

@davidbreitgand

Copy link
Copy Markdown
Contributor Author

thanks @davidbreitgand, what are your thoughts about implementing String() instead of MarshalJSON?

Thanks @nirrozenbaum. Actually, I considered String() :) But it doesn't address the actual failure mode here. The problem is that the broken log sites all go through structured logging, not %v like format.

It goes like logger.Info(..., "result", result) / "scoredModels", scored), which serializes values with encoding/json. And encoding/json does not consult fmt.Stringer. Rather it only looks at exported fields or a MarshalJSON method. So adding String() on *model would leave rendering {}.

The problem also shows up when datalayer.Model is nested inside another struct (PipelineRunResult.TargetModel, []ScoredModel). The outer struct is JSON-marshaled by the logger, so the child must implement MarshalJSON, because String() on the child is not called in that path.

MarshalJSON also preserves the structured shape ({"name":"..."}, {"name":"...","score":...}), which stays greppable and "jq-able" in log tooling. String() blob inside a JSON field would be actually clumsy for that.

Does this make sense to you?

Having said that, I'm happy to add String() in addition if you have %v format style call sites in mind, but it can't replace MarshalJSON for the reported bug. Do you have a specific site where String() would be the right fix in the scope of this PR?

Signed-off-by: David Breitgand <davidbreitgand@users.noreply.github.com>
@davidbreitgand

Copy link
Copy Markdown
Contributor Author

@nirrozenbaum can you please take another look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] datalayer.Model serializes as {} in structured logs in picker/selector

2 participants