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

Propagation: only warn about oversized baggage headers when headers exist #2212

Merged
merged 1 commit into from
Oct 16, 2021
Merged

Conversation

marier-nico
Copy link
Contributor

Description

Warnings were being logged about the baggage header being too long when the header was not present. This PR just adds a check to make sure the warning is only emitted when the header exists.

The main issue is that this was creating a lot of noise in logs, because the project where opentelemetry is used is the first one to receive requests (and thus will never have the baggage header set).

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Tox has been run. I'm not sure much else is necessary, given that no behavior has been changed, only logging.

  • Run tox

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@marier-nico marier-nico requested a review from a team October 15, 2021 20:21
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 15, 2021

CLA Signed

The committers are authorized under a signed CLA.

Copy link
Contributor

@owais owais left a comment

Choose a reason for hiding this comment

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

Looks weird to check for if not something and then immediate if something but approving :)

@marier-nico
Copy link
Contributor Author

@owais Yeah, I wasn't sure myself, I tried to go for the smallest change and that was probably not the best idea 😅

Do you think the following would be better?

if not header:
    return context

if len(header) > self._MAX_HEADER_LENGTH:
    _logger.warning(
        "Baggage header `%s` exceeded the maximum number of bytes per baggage-string",
        header,
    )
    return context

@owais
Copy link
Contributor

owais commented Oct 15, 2021

I feel your snippet is more clear and shows the intent of the function better. I'm fine either way.

@owais
Copy link
Contributor

owais commented Oct 15, 2021

Please add a changelog entry

This commit makes sure that warnings about the baggage header length are
only emitted when the header is actually present, since it does not make
sense to warn about a missing header's length.
@marier-nico
Copy link
Contributor Author

Should be all fixed! 😄

@owais owais requested a review from a team October 16, 2021 00:47
@owais owais enabled auto-merge (squash) October 16, 2021 01:22
@owais
Copy link
Contributor

owais commented Oct 16, 2021

Set to auto-merge once it receives a 2nd approval.

@owais owais merged commit 73230b6 into open-telemetry:main Oct 16, 2021
@debu999
Copy link

debu999 commented Oct 19, 2021

You are the savior Thanks a ton for fixing so early.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants