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

feat(instrumentation): update traces migration #405

Merged
merged 11 commits into from
Dec 23, 2022
Merged

feat(instrumentation): update traces migration #405

merged 11 commits into from
Dec 23, 2022

Conversation

mat-rumian
Copy link
Contributor

@mat-rumian mat-rumian commented Dec 15, 2022 •
edited
Loading

PR adds:

  • cascading_filter and source processors migration from v2 to v3.
  • information about migration details if any of tracing components was enabled
  • updates tracing-replaces check

@mat-rumian mat-rumian requested a review from a team as a code owner December 15, 2022 12:56
swiatekm
swiatekm reviewed Dec 15, 2022
View reviewed changes
Copy link

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

Can we put this in a new migration instead of modifying an existing one?

I'm also a bit confused. Why aren't we moving the whole config?

@mat-rumian
Copy link
Contributor Author

Can we put this in a new migration instead of modifying an existing one?

I've created another migration directory related only to config changes also cleaned up existing tracing-replaces migration.

I'm also a bit confused. Why aren't we moving the whole config?

Because tracesSampler will now take care only about sampling. otelcolInstrumentation will use some of the processors and exporters from tracesSampler but their configuration is "static". (e.g. resourcedetection or attributes processors). tracesGateway doesn't contain any customized configuration.

kkujawa-sumo
kkujawa-sumo reviewed Dec 23, 2022
View reviewed changes
src/go/cmd/update-collection-v3/migrations/tracing-config/migrate.go Outdated
)

func Migrate(yamlV2 string) (yamlV3 string, err error) {
valuesV2, err := parseValues(yamlV2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Now, we use the style in which we use input/output instead of V2/V3
some comments from other pull requests:
#394 (comment)
#398

Copy link
Contributor

Choose a reason for hiding this comment

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

🤷‍♀️

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to use new style in new files with code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Kasia, I've applied all the changes related to structs naming in every tracing-* migration.

@mat-rumian mat-rumian merged commit c03c8d6 into main Dec 23, 2022
@mat-rumian mat-rumian deleted the apply-instrumentation-migration branch December 23, 2022 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kkujawa-sumo kkujawa-sumo kkujawa-sumo approved these changes

@swiatekm swiatekm Awaiting requested review from swiatekm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mat-rumian @kkujawa-sumo @swiatekm