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 traceability_id field to alb log parse #862

Merged
merged 5 commits into from
Jun 3, 2024
Merged

Conversation

skbki
Copy link
Contributor

@skbki skbki commented May 29, 2024

Apparently AWS added new field to ALB log which broke our ingestion pipeline. Only place I found it is AWS Athena guide to parse ALB logs
https://docs.aws.amazon.com/athena/latest/ug/application-load-balancer-logs.html

Closes: #852

@jszwedko jszwedko requested a review from pront May 29, 2024 13:45
@misterek
Copy link

This is currently breaking all of our ALB log parsing.

However, is it possible to adjust so that new fields are ignored, not breaking everything? This is apparently being rolled out slowly region to region, so some amount of logs will be broken.

Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution @skbki !

I think the traceability_id field needs to be optional since it appears that AWS seems to still be rolling out this change and so there may be users where this field doesn't exist yet (or if they are processing older ALB logs). Your change here makes the field required.

I agree with #862 (comment) that we should also be conservative and just discard unknown fields to future-proof this function, but that could be handled separately from the PR simply adding support for traceability_id.

@skbki
Copy link
Contributor Author

skbki commented Jun 1, 2024

Good point. I've managed to make the field optional

Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

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

Thanks @skbki . This looks good to me!

Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

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

We'll just need a changelog entry too.

@jszwedko jszwedko enabled auto-merge June 3, 2024 17:03
@pront
Copy link
Contributor

pront commented Jun 3, 2024

Hi @skbki, thank you for this contribution! A cargo fmt should fix the failing CI check.

@jszwedko
Copy link
Member

jszwedko commented Jun 3, 2024

Clippy flagged an issue too (cargo clippy should show it locally).

auto-merge was automatically disabled June 3, 2024 20:20

Head branch was pushed to by a user without write access

@skbki
Copy link
Contributor Author

skbki commented Jun 3, 2024

Corrected issues, should be fine now

@jszwedko jszwedko enabled auto-merge June 3, 2024 20:40
@jszwedko jszwedko added this pull request to the merge queue Jun 3, 2024
Merged via the queue into vectordotdev:main with commit c7802b5 Jun 3, 2024
11 checks passed
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.

parse_aws_alb_log failing with new AWS field
4 participants