Skip to content

[chore][pprofiles] Fix pprofile DurationNano to be a TypeUint64 #44397

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

Jump to bottom
Merged
merged 13 commits into from
Nov 24, 2025
Merged

[chore][pprofiles] Fix pprofile DurationNano to be a TypeUint64 #44397

merged 13 commits into from
Nov 24, 2025
+22,191 −22,171

Conversation

@florianl
Copy link
Contributor

@florianl florianl commented Nov 19, 2025

Description

Complementary change to open-telemetry/opentelemetry-collector#14188.

Link to tracking issue

Fixes

Testing

Documentation

@florianl florianl mentioned this pull request Nov 19, 2025
Merged
douglascamata
douglascamata suggested changes Nov 19, 2025
View reviewed changes
douglascamata
douglascamata approved these changes Nov 20, 2025
View reviewed changes
Copy link
Member

@douglascamata douglascamata left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to fix this!

edmocosta
edmocosta reviewed Nov 20, 2025
View reviewed changes
pkg/ottl/contexts/internal/ctxprofile/profile.go Outdated
return ottl.StandardGetSetter[K]{
Getter: func(_ context.Context, tCtx K) (any, error) {
return tCtx.GetProfile().Duration().AsTime(), nil
return tCtx.GetProfile().DurationNano(), nil
Copy link
Contributor

@edmocosta edmocosta Nov 20, 2025

Choose a reason for hiding this comment

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

Suggested change
return tCtx.GetProfile().DurationNano(), nil
return time.Unix(0, int64(tCtx.GetProfile().DurationNano())).UTC(), nil

This should continue returning time.Time, like before.

Copy link
Contributor Author

@florianl florianl Nov 20, 2025

Choose a reason for hiding this comment

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

I don't think this is a good idea. time.Time represents a point in time and not a duration. That is the whole point of open-telemetry/opentelemetry-collector#14188.

Copy link
Contributor Author

@florianl florianl Nov 21, 2025

Choose a reason for hiding this comment

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

If Duration and its return time.Time should stay, what is the procedure to mark it as deprecated and direct people to use DurationNano?

Copy link
Contributor

@edmocosta edmocosta Nov 21, 2025

Choose a reason for hiding this comment

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

My suggestion is to keep it returning time.Time for now (even if not ideal), so we don't need to delay this PR. We can handle this field deprecation and removal later in a separate breaking change issue.

Copy link
Contributor Author

@florianl florianl Nov 24, 2025

Choose a reason for hiding this comment

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

Requiring multiple breaking changes sounds like a lot of overhead, not only for the collector-contrib side.
With 4dbce06 I have aligned the changes.

Copy link
Contributor

@edmocosta edmocosta Nov 26, 2025

Choose a reason for hiding this comment

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

My suggestion was to not break it in this PR, the applied changes seems to be a breaking change, which isn't clear to users reading the added change log.

github-merge-queue bot pushed a commit to open-telemetry/opentelemetry-collector that referenced this pull request Nov 20, 2025
@florianl
Fix pprofile DurationNano to be a TypeUint64 (#14188)
40c567e 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

pcommon.Timestamp better represents a point in time rather than a
duration. So keeping the proto field type for DurationNano is a better
fit.

Corresponding Collector-contrib change:
open-telemetry/opentelemetry-collector-contrib#44397

ping @open-telemetry/profiling-approvers 

<!-- Issue number if applicable -->
#### Link to tracking issue
Fixes #

<!--Describe what testing was performed and which tests were added.-->
#### Testing

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Signed-off-by: Florian Lehner <[email protected]>
VihasMakwana
VihasMakwana reviewed Nov 21, 2025
View reviewed changes
florianl
florianl commented Nov 21, 2025
View reviewed changes
pkg/ottl/contexts/internal/ctxprofile/profile.go Outdated
return ottl.StandardGetSetter[K]{
Getter: func(_ context.Context, tCtx K) (any, error) {
return tCtx.GetProfile().Duration().AsTime(), nil
return tCtx.GetProfile().DurationNano(), nil
Copy link
Contributor Author

@florianl florianl Nov 20, 2025

Choose a reason for hiding this comment

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

I don't think this is a good idea. time.Time represents a point in time and not a duration. That is the whole point of open-telemetry/opentelemetry-collector#14188.

pkg/ottl/contexts/internal/ctxprofile/profile.go Outdated
return ottl.StandardGetSetter[K]{
Getter: func(_ context.Context, tCtx K) (any, error) {
return tCtx.GetProfile().Duration().AsTime(), nil
return tCtx.GetProfile().DurationNano(), nil
Copy link
Contributor Author

@florianl florianl Nov 21, 2025

Choose a reason for hiding this comment

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

If Duration and its return time.Time should stay, what is the procedure to mark it as deprecated and direct people to use DurationNano?

@dmathieu dmathieu mentioned this pull request Nov 21, 2025
Closed
florianl and others added 2 commits November 24, 2025 08:08
@florianl
[chore] mod and toolchain tidy
83f9b84 
Signed-off-by: Florian Lehner <[email protected]>
@florianl
fix TestCompareProfiles
41392eb 
Signed-off-by: Florian Lehner <[email protected]>
mx-psi
mx-psi approved these changes Nov 24, 2025
View reviewed changes
@mx-psi mx-psi requested a review from edmocosta November 24, 2025 16:35
@mx-psi
Copy link
Member

mx-psi commented Nov 24, 2025

I am going to merge this to avoid merge conflicts.

@TylerHelmuth @evan-bradley @edmocosta I believe Florian addressed the OTTL asks satisfactorily but PTAL as codeowners!

@mx-psi mx-psi merged commit 0628469 into open-telemetry:main Nov 24, 2025
199 of 203 checks passed
@github-actions github-actions bot added this to the next release milestone Nov 24, 2025
@mx-psi mx-psi mentioned this pull request Nov 24, 2025
Closed
@github-actions github-actions bot mentioned this pull request Nov 24, 2025
Closed
@edmocosta
Copy link
Contributor

edmocosta commented Nov 26, 2025

I am going to merge this to avoid merge conflicts.

@TylerHelmuth @evan-bradley @edmocosta I believe Florian addressed the OTTL asks satisfactorily but PTAL as codeowners!

The merged OTTL changes are a braking change (changed a field type), and unfortunately we don't have a specific change log for that, which is not ideal, even considering the profile stability. I wish we could split this kind of PRs per components in the future, at least when meaningful code is modified.

@mx-psi
Copy link
Member

mx-psi commented Nov 26, 2025

I am going to merge this to avoid merge conflicts.
@TylerHelmuth @evan-bradley @edmocosta I believe Florian addressed the OTTL asks satisfactorily but PTAL as codeowners!

The merged OTTL changes are a braking change (changed a field type), and unfortunately we don't have a specific change log for that, which is not ideal, even considering the profile stability.

Wouldn't an API changelog work for that?

I wish we could split this kind of PRs per components in the future, at least when meaningful code is modified.

I am supportive of this, feel free to block the PR in the future if you'd rather the PR not be merged

@edmocosta
Copy link
Contributor

edmocosta commented Nov 26, 2025

Wouldn't an API changelog work for that?

That's an user breaking change, the previous duration field type was time.Time, and it was changed to int64. Usages of that field on OTTL statement can possibly break the collector config validation.

I'm on PTO and will only have access to my computer next week, if someone else could apply this suggestion or add the change log, that would be great. @TylerHelmuth @evan-bradley @florianl?

Thank you!

@mx-psi mx-psi mentioned this pull request Nov 26, 2025
Closed
@mx-psi
Copy link
Member

mx-psi commented Nov 26, 2025

I misunderstood your point, sorry. I created a release blocker issue based on your comment to add the entry, see #44560

florianl added a commit to florianl/opentelemetry-collector-contrib that referenced this pull request Nov 27, 2025
@florianl
[chore] add changelog for open-telemetry#44397
8441bff 
Fixes open-telemetry#44560

Signed-off-by: Florian Lehner <[email protected]>
@florianl florianl mentioned this pull request Nov 27, 2025
Merged
atoulme pushed a commit that referenced this pull request Nov 27, 2025
@florianl
[chore] add changelog for #44397 (#44571)
f438958 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Add additional changelog for a breaking change introduced by #44397.


<!-- Issue number (e.g. #1234) or full URL to issue, if applicable. -->
#### Link to tracking issue

Fixes
#44560

<!--Describe what testing was performed and which tests were added.-->
#### Testing

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Signed-off-by: Florian Lehner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@VihasMakwana VihasMakwana VihasMakwana left review comments

@mx-psi mx-psi mx-psi approved these changes

@fatsheep9146 fatsheep9146 Awaiting requested review from fatsheep9146 fatsheep9146 is a code owner

@TylerHelmuth TylerHelmuth Awaiting requested review from TylerHelmuth TylerHelmuth is a code owner

@evan-bradley evan-bradley Awaiting requested review from evan-bradley evan-bradley is a code owner

@atingchen atingchen Awaiting requested review from atingchen

@dao-jun dao-jun Awaiting requested review from dao-jun

@iblancasa iblancasa Awaiting requested review from iblancasa

@povilasv povilasv Awaiting requested review from povilasv

@MovieStoreGuy MovieStoreGuy Awaiting requested review from MovieStoreGuy MovieStoreGuy is a code owner

@andrzej-stencel andrzej-stencel Awaiting requested review from andrzej-stencel andrzej-stencel is a code owner

@crobert-1 crobert-1 Awaiting requested review from crobert-1 crobert-1 is a code owner

@atoulme atoulme Awaiting requested review from atoulme atoulme is a code owner

@pjanotti pjanotti Awaiting requested review from pjanotti pjanotti is a code owner

@dashpole dashpole Awaiting requested review from dashpole dashpole is a code owner

@dmitryax dmitryax Awaiting requested review from dmitryax dmitryax is a code owner

@dehaansa dehaansa Awaiting requested review from dehaansa dehaansa is a code owner

@ChrsMark ChrsMark Awaiting requested review from ChrsMark ChrsMark is a code owner

@ArthurSens ArthurSens Awaiting requested review from ArthurSens ArthurSens is a code owner

@axw axw Awaiting requested review from axw axw is a code owner

@mwear mwear Awaiting requested review from mwear mwear is a code owner

@codeboten codeboten Awaiting requested review from codeboten codeboten is a code owner

@braydonk braydonk Awaiting requested review from braydonk braydonk is a code owner

@songy23 songy23 Awaiting requested review from songy23 songy23 is a code owner

@jsuereth jsuereth Awaiting requested review from jsuereth jsuereth is a code owner

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu bogdandrutu is a code owner

@edmocosta edmocosta Awaiting requested review from edmocosta edmocosta is a code owner

+1 more reviewer

@douglascamata douglascamata douglascamata approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mwear mwear

Labels

cmd/golden cmd/opampsupervisor cmd/otelcontribcol otelcontribcol command cmd/oteltestbedcol cmd/telemetrygen telemetrygen command confmap/provider/aesprovider confmap/provider/googlesecretmanagerprovider confmap/provider/s3provider confmap/provider/secretsmanagerprovider connector/count connector/datadog connector/exceptions connector/failover connector/grafanacloud connector/metricsaslogs connector/otlpjson connector/roundrobin connector/routing connector/servicegraph connector/signaltometrics connector/slowsql connector/spanmetrics connector/sum exporter/alertmanager exporter/alibabacloudlogservice Alibaba components exporter/awscloudwatchlogs awscloudwatchlogs exporter exporter/awsemf awsemf exporter exporter/awskinesis exporter/awss3 exporter/awsxray exporter/azureblob exporter/azuredataexplorer exporter/azuremonitor exporter/bmchelix exporter/carbon exporter/cassandra exporter/clickhouse exporter/coralogix exporter/datadog Datadog components exporter/dataset exporter/doris exporter/elasticsearch exporter/faro exporter/file exporter/googlecloud exporter/googlecloudpubsub exporter/googlecloudstorage exporter/googlemanagedprometheus Google Managed Prometheus exporter exporter/honeycombmarker exporter/influxdb exporter/kafka exporter/loadbalancing exporter/logicmonitor exporter/logzio exporter/mezmo exporter/opensearch exporter/otelarrow exporter/prometheus exporter/prometheusremotewrite exporter/pulsar exporter/rabbitmq exporter/sapm exporter/sematext exporter/sentry exporter/signalfx exporter/splunkhec exporter/stef exporter/sumologic exporter/syslog exporter/tencentcloudlogservice exporter/tinybird exporter/zipkin extension/ack extension/asapauth extension/awsproxy extension/azureauth extension/basicauth extension/bearertokenauth extension/cgroupruntime extension/datadog extension/encoding/avrologencoding extension/encoding/awscloudwatchmetricstreams extension/encoding/awslogsencoding extension/encoding/googlecloudlogentryencoding extension/encoding/jaegerencoding extension/encoding/jsonlogencoding extension/encoding/otlpencoding extension/encoding/skywalkingencoding extension/encoding/textencoding extension/encoding/zipkinencoding extension/encoding extension/googleclientauth extension/headerssetter extension/healthcheck Health Check Extension extension/healthcheckv2 pkg/ottl pkg/pdatatest pkg/translator/pprof

Projects

None yet

Milestone

next release

Development

Successfully merging this pull request may close these issues.

6 participants

@florianl @mx-psi @edmocosta @douglascamata @VihasMakwana @mwear