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

[RedisReceiver] Add service.instance.id as an additional resource attribute #22046

Closed
wants to merge 2 commits into from

Conversation

hughesjj
Copy link
Contributor

@hughesjj hughesjj commented May 17, 2023

Description:
Add a resource_attribute to track service.instance.id and set the value of such to be the configuration's endpoint

Link to tracking Issue:
#22044

Testing:
Ran test in intellij

Documentation:
I believe mdatagen takes care of this but can add to README too. LMK if you think I should.

@hughesjj hughesjj requested a review from a team May 17, 2023 16:50
@hughesjj hughesjj requested a review from dmitryax as a code owner May 17, 2023 16:50
@github-actions github-actions bot added the receiver/redis Redis related issues label May 17, 2023
Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

This looks good, one nit on the changelog message.

component: "redisreceiver"

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Adds service.instance.id as a resource parameter to redis receiver.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
note: Adds service.instance.id as a resource parameter to redis receiver.
note: Adds service.instance.id as an additional resource attribute (disabled by default) to redis receiver.

Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

This looks good, one nit on the changelog message.

@dmitryax
Copy link
Member

dmitryax commented May 17, 2023

@hughesjj thanks for fixing some issues along the way! Can you please submit them in different PRs to keep the git history clean? And I believe all of them deserve separate changelog entries

@hughesjj
Copy link
Contributor Author

@dmitryax Yeah, sure! How about one for the bugfixes (removing redis:// and actually honoring the enabled flag), and another one for the new resource attribute?

I could further split up the proposed "bugfixes" PR into different PRs too, nbd either way, lmk

@dmitryax
Copy link
Member

The best practice is to apply each change in a separate PR especially if they are independent

@atoulme
Copy link
Contributor

atoulme commented May 19, 2023

Please rebase

@hughesjj hughesjj force-pushed the redis_add_instanceid branch from 7d3027e to 60ef2a3 Compare May 22, 2023 18:07
@hughesjj hughesjj force-pushed the redis_add_instanceid branch from 60ef2a3 to b75aae2 Compare May 30, 2023 23:02
@dmitryax
Copy link
Member

Please update the PR description

@hughesjj hughesjj marked this pull request as draft May 31, 2023 00:29
@hughesjj hughesjj changed the title Improve Redis resource attribute adherence [RedisReceiver] Add service.instance.id as an additional resource attribute May 31, 2023
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jun 14, 2023
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
receiver/redis Redis related issues Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants