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

Add local and tag correlation fields #37435

Conversation

adispezo
Copy link
Contributor

@adispezo adispezo commented Sep 16, 2023

Currently with the following config example it's not possible to use BaggageField.getByName, BaggageField.updateValue for example.

management:
    tracing:
        baggage:
            correlation:
                fields: 
                    - some_local_field
                    - some_remote_field
           remote-fields:
               - some_remote_field

Only the "some_remote_field" can be retrieved and updated.

With the code proposed also the "local" correlation fields will be accessible.

@pivotal-cla
Copy link

@adispezo Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@adispezo Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 16, 2023
@adispezo adispezo closed this Sep 16, 2023
@adispezo adispezo reopened this Sep 16, 2023
@mhalbritter
Copy link
Contributor

Hello,

thanks for the PR! I've added some comments.

@mhalbritter mhalbritter added the status: waiting-for-feedback We need additional information before we can continue label Sep 18, 2023
@mhalbritter mhalbritter self-assigned this Sep 18, 2023
@mhalbritter mhalbritter changed the title Add local correlation fields to baggageFields Add local correlation fields for Brave Sep 18, 2023
@mhalbritter
Copy link
Contributor

Hey @adispezo , thanks for the changes! I'll get some feedback from the observability team on the local field change in Brave and then come back to you.

@mhalbritter
Copy link
Contributor

Looks good, thank you!

Note to the boot team: when merging, we need something like this for OpenTelemetry, too.

@mhalbritter mhalbritter added type: enhancement A general enhancement for: merge-with-amendments Needs some changes when we merge and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Sep 25, 2023
@mhalbritter mhalbritter added this to the 3.x milestone Sep 25, 2023
@marcingrzejszczak
Copy link
Contributor

There's no way to do it easily with OTel. What currently is being done is that all baggage gets extracted / injected (e.g.https://github.com/open-telemetry/opentelemetry-java/blob/main/api/all/src/main/java/io/opentelemetry/api/baggage/propagation/W3CBaggagePropagator.java#L50). We would need to wrap their baggage propagation mechanism into our one that would modify the current baggage not to contain local keys and then delegate to their code.

@mhalbritter
Copy link
Contributor

mhalbritter commented Oct 2, 2023

Hm :( In this case, we won't support local fields when using OpenTelemetry. We could issue a log WARN statement if local fields are configured and OTel is used.

mhalbritter pushed a commit to mhalbritter/spring-boot that referenced this pull request Oct 2, 2023
mhalbritter added a commit to mhalbritter/spring-boot that referenced this pull request Oct 2, 2023
mhalbritter added a commit to mhalbritter/spring-boot that referenced this pull request Oct 2, 2023
@mhalbritter
Copy link
Contributor

I have some polished changes and tagged fields for Otel here: https://github.com/mhalbritter/spring-boot/tree/pr/37435

@mhalbritter mhalbritter modified the milestones: 3.x, 3.3.x Nov 7, 2023
@mhalbritter
Copy link
Contributor

Before merging this, #38724 needs to be considered.

mhalbritter pushed a commit that referenced this pull request Jan 9, 2024
Local fields only work in Brave and not with OpenTelemetry.

Tagged fields work both with Brave and with OpenTelemetry.

See gh-37435
mhalbritter added a commit that referenced this pull request Jan 9, 2024
@mhalbritter mhalbritter modified the milestones: 3.3.x, 3.3.0-M1 Jan 9, 2024
@mhalbritter mhalbritter changed the title Add local correlation fields for Brave Add local and tag correlation fields Jan 9, 2024
@mhalbritter
Copy link
Contributor

Merged, thank you. To accommodate the changes from #38724, i had to refactor a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants