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.

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

[processor/k8sattributes] Deprecate FieldExtractConfig.Regex config option #33411

Merged
merged 19 commits into from
Jul 29, 2024
Merged

[processor/k8sattributes] Deprecate FieldExtractConfig.Regex config option #33411

merged 19 commits into from
Jul 29, 2024

Conversation

odubajDT
Copy link
Contributor

@odubajDT odubajDT commented Jun 6, 2024

Link to tracking Issue: #25128

@github-actions github-actions bot added the processor/k8sattributes k8s Attributes processor label Jun 6, 2024
@odubajDT odubajDT marked this pull request as ready for review June 6, 2024 13:30
@odubajDT odubajDT requested a review from a team June 6, 2024 13:30
crobert-1
crobert-1 reviewed Jun 6, 2024
View reviewed changes
Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

I think this would be a good time to update the README as well. There are some configuration examples using what will now be a deprecated option. I'd suggest we may need to add a temporary section on moving to the recommended configuration approach from the deprecated config (to help users during upgrade), and then also update the current examples to use the recommended approach.

.chloggen/chore_25128_regex.yaml Outdated Show resolved Hide resolved
.chloggen/chore_25128_regex.yaml Outdated Show resolved Hide resolved
processor/k8sattributesprocessor/config.go Outdated Show resolved Hide resolved
.chloggen/chore_25128_regex.yaml Outdated Show resolved Hide resolved
crobert-1
crobert-1 reviewed Jun 7, 2024
View reviewed changes
Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Minor wording and formatting nits

.chloggen/chore_25128_regex.yaml Outdated Show resolved Hide resolved
.chloggen/chore_25128_regex.yaml Outdated Show resolved Hide resolved
evan-bradley
evan-bradley previously approved these changes Jun 10, 2024
View reviewed changes
processor/k8sattributesprocessor/config.go Show resolved Hide resolved
.chloggen/chore_25128_regex.yaml Outdated Show resolved Hide resolved
@evan-bradley evan-bradley dismissed their stale review June 10, 2024 16:00

Hit the wrong button

TylerHelmuth
TylerHelmuth reviewed Jun 12, 2024
View reviewed changes
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

Before we move forward with the deprecation I'd like to see a solution to #25128 (comment)

@evan-bradley
Copy link
Contributor

@TylerHelmuth The snippet you posted doesn't make use of the regex field being deprecated in this PR and shouldn't look any different after this change. Is there a relationship between the key_regex field and regex field that will impact deprecating only the regex field?

@TylerHelmuth
Copy link
Member

@evan-bradley you're right I mixed up the 2 fields.

evan-bradley
evan-bradley reviewed Jun 21, 2024
View reviewed changes
processor/k8sattributesprocessor/config.go Outdated Show resolved Hide resolved
processor/k8sattributesprocessor/config.go Outdated Show resolved Hide resolved
processor/k8sattributesprocessor/config.go Outdated Show resolved Hide resolved
processor/k8sattributesprocessor/README.md Show resolved Hide resolved
.chloggen/chore_25128_regex.yaml Outdated Show resolved Hide resolved
@odubajDT odubajDT force-pushed the chore/25128/regex branch 2 times, most recently from 263b5c5 to 7747698 Compare July 2, 2024 08:29
evan-bradley
evan-bradley approved these changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Looks good to me overall. @dmitryax @fatsheep9146 @rmfitzpatrick @TylerHelmuth please take a look.

processor/k8sattributesprocessor/config.go Outdated Show resolved Hide resolved
processor/k8sattributesprocessor/config.go Outdated Show resolved Hide resolved
.chloggen/chore_25128_regex.yaml Outdated Show resolved Hide resolved
processor/k8sattributesprocessor/config.go Outdated Show resolved Hide resolved
@odubajDT odubajDT force-pushed the chore/25128/regex branch 4 times, most recently from b02cb87 to 583c404 Compare July 18, 2024 13:43
odubajDT and others added 5 commits July 19, 2024 07:30
@odubajDT
[processor/k8sattributes] Deprecate FieldExtractConfig.Regex config o…
946223e 
…ption

Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT @crobert-1
Apply suggestions from code review
92a217a 
Co-authored-by: Curtis Robert <crobert@splunk.com>
@odubajDT
pr review
1095345 
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT
fix chlog
ded6b69 
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT
fix chlog
27373ab 
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
odubajDT and others added 10 commits July 19, 2024 07:30
@odubajDT @crobert-1
Apply suggestions from code review
6652d6d 
Co-authored-by: Curtis Robert <crobert@splunk.com>
@odubajDT @evan-bradley
Update .chloggen/chore_25128_regex.yaml
55f52dc 
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
@odubajDT
add feature gate
0c503fd 
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT @evan-bradley
Apply suggestions from code review
82b3bfd 
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
@odubajDT
add example migration in documentation
cbd1fcb 
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT @evan-bradley
Update processor/k8sattributesprocessor/README.md
791361d 
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
@odubajDT
minor polishing
61437ff 
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT
polish test
0ec553c 
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT @evan-bradley
Apply suggestions from code review
429973b 
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
@odubajDT
fix after rebase
5d92b35 
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT odubajDT force-pushed the chore/25128/regex branch from 583c404 to 5d92b35 Compare July 19, 2024 05:30
evan-bradley and others added 2 commits July 22, 2024 16:58
@evan-bradley @TylerHelmuth
Update versions after release
2a584c3 
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
@evan-bradley
Merge branch 'main' into chore/25128/regex
974f371
@evan-bradley evan-bradley merged commit 1a8bdfc into open-telemetry:main Jul 29, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 29, 2024
@odubajDT odubajDT mentioned this pull request Aug 26, 2024
Closed
6 tasks
@ChrsMark ChrsMark mentioned this pull request Nov 15, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crobert-1 crobert-1 crobert-1 left review comments

@evan-bradley evan-bradley evan-bradley approved these changes

@TylerHelmuth TylerHelmuth TylerHelmuth approved these changes

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

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

@rmfitzpatrick rmfitzpatrick Awaiting requested review from rmfitzpatrick

Assignees

@atoulme atoulme

Labels
processor/k8sattributes k8s Attributes processor
Projects
None yet
Milestone
v0.106.0
Development

Successfully merging this pull request may close these issues.
5 participants
@odubajDT @evan-bradley @TylerHelmuth @crobert-1 @atoulme