Skip to content
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.

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

Timeline #55

Merged
merged 7 commits into from
Sep 10, 2024
Merged

Timeline #55

merged 7 commits into from
Sep 10, 2024

Conversation

r1viollet
Copy link
Collaborator

Description

Allow timestamps to be added to the samples as labels.

Additional information

I added the process name as a label as it makes it easier to de-duplicate, rather than using the frame information.
We can re-visit this idea.

@r1viollet r1viollet requested a review from a team as a code owner September 3, 2024 10:12
@r1viollet r1viollet force-pushed the r1viollet/rebase-timeline branch 2 times, most recently from 46acd87 to 8496397 Compare September 3, 2024 15:41
totalSampleCount += len(traceInfo.timestamps)
sample := &pprofile.Sample{}
count := len(traceInfo.timestamps)
r.processSample(sample, profile, &traceKey, traceInfo, fileIDtoMapping,

Choose a reason for hiding this comment

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

🟠 Code Vulnerability

Potential memory range aliasing. Avoid using the memory reference. (...read more)

Implicit memory aliasing in for loops refers to a scenario in Go programming when two or more pointers reference the same location in memory, creating unexpected side effects. This often results in a common mistake amongst Go programmers due to the 'range' clause.

Consider this example, where a slice of pointers is created in a loop:

data := []int{1, 2, 3}
pointers := make([]*int, 3)
for i, v := range data {
    pointers[i] = &v
}

You might expect the 'pointers' slice to hold addresses of elements in 'data' slice, but that's not the case. In each iteration of the loop, variable 'v' gets a new value but its memory address doesn't change because it's a loop variable. As a result, each element in 'pointers' slice points to the same memory location - the address of the loop variable 'v'. The final value of 'v' is '3', and since all pointers point to 'v', dereferencing the pointers would yield '3' regardless of the pointer's index in the slice.

To avoid implicit memory aliasing in for loops in Go, you should address the actual elements in the original data structure, like so:

data := []int{1, 2, 3}
pointers := make([]*int, 3)
for i := range data {
    pointers[i] = &data[i]
}

In this example, each pointer in the 'pointers' slice correctly points to the respective element in the 'data' slice.

Learn More

View in Datadog  Leave us feedback  Documentation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will silence this if we agree it is not an issue.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is not an issue thanks to https://go.dev/blog/loopvar-preview, and since go.mod has 1.22+ as the version https://github.com/DataDog/otel-profiling-agent/blob/main/go.mod#L3 this should be alright

@@ -559,12 +571,11 @@ func createPprofFunctionEntry(funcMap map[funcInfo]*pprofile.Function,
return function
}

//nolint:gocritic
func addTraceLabels(labels map[string][]string, k traceAndMetaKey) {
func addTraceLabels(labels map[string][]string, k *traceAndMetaKey,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed the traceAndMetaKey to a pointer, highlighting in case there was any thought put into this earlier

Copy link
Member

Choose a reason for hiding this comment

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

I initially kept it as a variable to avoid having to deal with #55 (comment) since this code is not on the hot path anyway for the copy to be too expensive (only called once per minute). Also wanted to keep the code closer to the upstream OTLP reporter. But this works for me as well, no strong opinions 👍

ah "github.com/elastic/otel-profiling-agent/armhelpers"
"github.com/elastic/otel-profiling-agent/libpf"
Copy link
Collaborator

@nsavoire nsavoire Sep 4, 2024

Choose a reason for hiding this comment

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

🤓 nitpick: ‏ This does not seem to belong to the proposed change.

if r.timeline {
timestamps = traceInfo.timestamps
}
addTraceLabels(sample.Label, &traceKey, baseExec, timestamps)

Choose a reason for hiding this comment

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

🟠 Code Vulnerability

Potential memory range aliasing. Avoid using the memory reference. (...read more)

Implicit memory aliasing in for loops refers to a scenario in Go programming when two or more pointers reference the same location in memory, creating unexpected side effects. This often results in a common mistake amongst Go programmers due to the 'range' clause.

Consider this example, where a slice of pointers is created in a loop:

data := []int{1, 2, 3}
pointers := make([]*int, 3)
for i, v := range data {
    pointers[i] = &v
}

You might expect the 'pointers' slice to hold addresses of elements in 'data' slice, but that's not the case. In each iteration of the loop, variable 'v' gets a new value but its memory address doesn't change because it's a loop variable. As a result, each element in 'pointers' slice points to the same memory location - the address of the loop variable 'v'. The final value of 'v' is '3', and since all pointers point to 'v', dereferencing the pointers would yield '3' regardless of the pointer's index in the slice.

To avoid implicit memory aliasing in for loops in Go, you should address the actual elements in the original data structure, like so:

data := []int{1, 2, 3}
pointers := make([]*int, 3)
for i := range data {
    pointers[i] = &data[i]
}

In this example, each pointer in the 'pointers' slice correctly points to the respective element in the 'data' slice.

Learn More

View in Datadog  Leave us feedback  Documentation

Allow timestamps to be added to the samples as labels.
With pprof this implies duplicating the samples.
- Add a timeline flag
- Ensure process name is added as a label (to help with grouping)
Add the list of timestamps to the sample's labels
Avoid a redundant nil check
@r1viollet r1viollet merged commit 054e5e3 into main Sep 10, 2024
15 checks passed
@r1viollet r1viollet deleted the r1viollet/rebase-timeline branch September 10, 2024 15:55
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.

3 participants