Skip to content

[processor/resourcedetection] Fix heroku config interface #24355

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

Merged
merged 1 commit into from
Jul 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 23 additions & 0 deletions .chloggen/resource-detection-processor-fix-heroku-attr.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# Use this changelog template to create an entry for release notes.
# If your change doesn't affect end users, such as a test fix or a tooling change,
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: bug_fix

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: processor/resourcedetection

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Fix Heroku config option for the `service.name` and `service.version` attributes

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [24355]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext: |
`service.name` and `service.version` attributes were mistakenly controlled by `heroku.app.name` and
`heroku.release.version` options under `resource_attributes` configuration introduced in 0.81.0.
This PR fixes the issue by using the correct config options named the same as the attributes.
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func (d *detector) Detect(_ context.Context) (resource pcommon.Resource, schemaU
attrs.PutStr(herokuAppID, v)
}
}
if d.resourceAttributes.HerokuAppName.Enabled {
if d.resourceAttributes.ServiceName.Enabled {
if v, ok := os.LookupEnv("HEROKU_APP_NAME"); ok {
attrs.PutStr(conventions.AttributeServiceName, v)
}
Expand All @@ -73,7 +73,7 @@ func (d *detector) Detect(_ context.Context) (resource pcommon.Resource, schemaU
attrs.PutStr(herokuReleaseCreationTimestamp, v)
}
}
if d.resourceAttributes.HerokuReleaseVersion.Enabled {
if d.resourceAttributes.ServiceVersion.Enabled {
if v, ok := os.LookupEnv("HEROKU_RELEASE_VERSION"); ok {
attrs.PutStr(conventions.AttributeServiceVersion, v)
}
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -5,33 +5,33 @@ all_set:
enabled: true
heroku.app.id:
enabled: true
heroku.app.name:
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this was broken behavior, but would it make sense to transition out this option since removing it is a breaking change?

Copy link
Member Author

@dmitryax dmitryax Jul 17, 2023

Choose a reason for hiding this comment

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

Like keeping it as no-op with a warning for a couple of versions? It'll be just a few manual changes to support that. Resource attributes don't yet have this if_enabled_set stuff. I was actually working towards adding that when I found this issue. I don't believe anyone applied this since it was introduced only last version. If you and @TylerHelmuth think that it's worth the effort, I can apply the suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it was only applied in the last release, you're right that it's unlikely to be used, so I'm okay breaking it. The closest thing I can find that would document what we should do here is the stability guarantees for scraping receivers, which only says we SHOULD follow the transition process for beta components.

enabled: true
heroku.dyno.id:
enabled: true
heroku.release.commit:
enabled: true
heroku.release.creation_timestamp:
enabled: true
heroku.release.version:
enabled: true
service.instance.id:
enabled: true
service.name:
enabled: true
service.version:
enabled: true
none_set:
resource_attributes:
cloud.provider:
enabled: false
heroku.app.id:
enabled: false
heroku.app.name:
enabled: false
heroku.dyno.id:
enabled: false
heroku.release.commit:
enabled: false
heroku.release.creation_timestamp:
enabled: false
heroku.release.version:
enabled: false
service.instance.id:
enabled: false
service.name:
enabled: false
service.version:
enabled: false
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@ resource_attributes:
description: The heroku.app.id
enabled: true
type: string
heroku.app.name:
description: The heroku.app.name
enabled: true
type: string
heroku.dyno.id:
description: The heroku.dyno.id
enabled: true
Expand All @@ -27,11 +23,15 @@ resource_attributes:
description: The heroku.release.creation_timestamp
enabled: true
type: string
heroku.release.version:
description: The heroku.release.version
enabled: true
type: string
service.instance.id:
description: The service.instance.id
type: string
enabled: true
enabled: true
service.name:
description: Heroku app name recorded as service.name.
enabled: true
type: string
service.version:
description: Heroku relese version set as service.version.
enabled: true
type: string