Skip to content

reporter: Do not generate unique dummy mappings #598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
christos68k merged 2 commits into from
Jul 14, 2025
Merged

reporter: Do not generate unique dummy mappings #598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f15b10f
reporter: do not generate unique dummy mappings
christos68k Jul 11, 2025
3350320
pdata: remove zero element from function_table
christos68k Jul 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 7 additions & 12 deletions reporter/internal/pdata/generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ const (
FrameMapLifetime = 1 * time.Hour
)

// DummyFileID is used as the FileID for a dummy mapping
var dummyFileID = libpf.NewFileID(0, 0)

// Generate generates a pdata request out of internal profiles data, to be
// exported.
func (p *Pdata) Generate(tree samples.TraceEventsTree,
Expand All @@ -42,7 +45,8 @@ func (p *Pdata) Generate(tree samples.TraceEventsTree,

// By specification, the first element should be empty.
stringSet.Add("")
funcSet.Add(funcInfo{nameIdx: stringSet.Add(""), fileNameIdx: stringSet.Add("")})
mappingSet.Add(dummyFileID)
dic.MappingTable().AppendEmpty()
Comment on lines +48 to +49
Copy link
Copy Markdown
Contributor

@rockdaboot rockdaboot Jul 11, 2025

Choose a reason for hiding this comment

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

Should we do the same for the link table right in this PR?
Because the .proto says

// link_table[0] must always be set to a zero value default link,
// so that _index fields can use 0 to indicate null/unset.

Copy link
Copy Markdown
Member Author

@christos68k christos68k Jul 11, 2025

Choose a reason for hiding this comment

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

There are no links being set in the current code so we shouldn't explicitly add a zero value in this case as the entire field is optional as per proto3.

Copy link
Copy Markdown
Contributor

@rockdaboot rockdaboot Jul 11, 2025

Choose a reason for hiding this comment

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

The documentation says "must always be", that's pretty clear to me.
Also there is validator code in pdatatest that validates if a given profile is valid or not. In the future, it will consider a message invalid if the default entry is missing.
Adding the default link value won't hurt, performance or overhead wise.

Copy link
Copy Markdown
Member Author

@christos68k christos68k Jul 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We should clarify this in the next SIG, I think we've previously discussed it when we talked about how to handle 'optional'. I don't think that the 'must always be' wording in the documentation overrides the optional nature of proto3 fields as it makes no sense to add an element to a field that shouldn't be there.

In my view, a validator would first check if the field is present and then perform checks on that field. If there are no links in the message, then a message with a missing link_table should pass validation. If there are links in the message, then the link_table should be present and obey the documented semantics.

Copy link
Copy Markdown
Contributor

@rockdaboot rockdaboot Jul 11, 2025

Choose a reason for hiding this comment

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

In my view, a validator would first check if the field is present and then perform checks on that field. If there are no links in the message, then a message with a missing link_table should pass validation. If there are links in the message, then the link_table should be present and obey the documented semantics.

That makes things unnecessarily complex and error-prone. Insisting on having a default value without "thinking" thus makes sense to me.

optional nature of proto3 fields

The proto doesn't use the optional keyword any more. So an optional index boils down to have a zero index value and thus requires a default table entry. For example

  // Reference to link in ProfilesDictionary.link_table. [optional]
  // It can be unset / set to 0 if no link exists, as link_table[0] is always a 'null' default value.
  int32 link_index = 5;

Copy link
Copy Markdown
Member Author

@christos68k christos68k Jul 11, 2025

Choose a reason for hiding this comment

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

We should clarify and not address this in this PR (which is about something else entirely).

Copy link
Copy Markdown
Contributor

@rockdaboot rockdaboot Jul 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

SGTM. Just suggested it as it would be a two-line change and the docs seemed to be clear.


for containerID, originToEvents := range tree {
if len(originToEvents) == 0 {
Expand Down Expand Up @@ -216,17 +220,8 @@ func (p *Pdata) setProfile(
}
locInfo.functionIndex = funcSet.Add(fi)
}

idx, exists := mappingSet.AddWithCheck(traceInfo.Files[i])
locInfo.mappingIndex = idx
if !exists {
// To be compliant with the protocol, generate a dummy mapping entry.
mapping := dic.MappingTable().AppendEmpty()
mapping.SetFilenameStrindex(stringSet.Add(""))
attrMgr.AppendOptionalString(mapping.AttributeIndices(),
semconv.ProcessExecutableBuildIDHtlhashKey,
traceInfo.Files[i].StringNoQuotes())
}
// mapping_table[0] is always the dummy mapping
locInfo.mappingIndex = 0
Comment thread
rockdaboot marked this conversation as resolved.
} // End frame type switch

idx, exists := locationSet.AddWithCheck(locInfo)
Expand Down
4 changes: 2 additions & 2 deletions reporter/internal/pdata/generate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ func TestFunctionTableOrder(t *testing.T) {
executables: map[libpf.FileID]samples.ExecInfo{},
frames: map[libpf.FileID]map[libpf.AddressOrLineno]samples.SourceInfo{},
events: map[libpf.Origin]samples.KeyToEventMapping{},
wantFunctionTable: []string{""},
wantFunctionTable: []string{},
expectedResourceProfiles: 0,
}, {
name: "single executable",
Expand Down Expand Up @@ -177,7 +177,7 @@ func TestFunctionTableOrder(t *testing.T) {
},
},
wantFunctionTable: []string{
"", "func1", "func2", "func3", "func4", "func5",
"func1", "func2", "func3", "func4", "func5",
},
},
} {
Expand Down