[extension/awslogs_encoding] Add support for VPC flow logs in plain text file format #38897

constanca-m · 2025-03-24T10:35:34Z

Description

Add support for VPC flow logs sent to S3 in plain text format.

Link to tracking issue

Fixes #38896.

Testing

There are new unit tests added.

Documentation

Comments in the code and unit tests should be enough.

Fields are mapped this way:

Flow log field	Attribute	Available?	Currently supported?
`version`	`aws.vpc.flow.log.version`	🔴
`account-id`	`cloud.account.id`	🟢
`interface-id`	`aws.eni.id`	🔴
`srcaddr`	`source.address` or `network.peer.address`	🔴
`pkt-srcaddr`	`source.address`	🟢
`dstaddr`	`destination.address` or `network.peer.address`	🔴
`pkt-dstaddr`	`destination.address`	🟢
`srcport`	`source.port`	🟢
`dstport`	`destination.port`	🟢
`protocol`	`network.protocol.name`	🟢
`packets`	`aws.vpc.flow.packets`	🔴
`bytes`	`aws.vpc.flow.bytes`	🔴
`start`	`aws.vpc.flow.start`	🔴
`end`	log timestamp	N/A
`action`	`aws.vpc.flow.action`	🔴
`log-status`	`aws.vpc.flow.status`	🔴
`vpc-id`	`aws.vpc.id`	🔴
`subnet-id`	`aws.vpc.subnet.id`	🔴
`instance-id`	`host.id`	🔴
`tcp-flags`	`network.tcp.flags`	🔴
`type`	`network.type`	🟢
`region`	`cloud.region`	🟢
`az-id`	`aws.az.id`	🔴
`sublocation-type`	`aws.sublocation.type`	🔴
`sublocation-id`	`aws.sublocation.id`	🔴
`pkt-src-aws-service`	`aws.vpc.flow.source.service`	🔴
`pkt-dst-aws-service`	`aws.vpc.flow.destination.service`	🔴
`flow-direction`	`network.io.direction`	🟢
`traffic-path`	`aws.vpc.flow.traffic_path`	🔴
`ecs-cluster-arn`	`aws.ecs.cluster.arn`	🟢	🔴
`ecs-cluster-name`	`aws.ecs.cluster.name`	🔴	🔴
`ecs-container-instance-arn`	`aws.ecs.container.instance.arn`	🔴	🔴
`ecs-container-instance-id`	`aws.ecs.container.instance.id`	🔴	🔴
`ecs-container-id`	`aws.ecs.container.id`	🔴	🔴
`ecs-second-container-id`	`aws.ecs.second.container.arn`	🔴	🔴
`ecs-service-name`	`aws.ecs.service.name`	🔴	🔴
`ecs-task-definition-arn`	`aws.ecs.task.definition.arn`	🔴	🔴
`ecs-task-arn`	`aws.ecs.task.arn`	🟢	🔴
`ecs-task-id`	`aws.ecs.task.id`	🟢	🔴
`reject-reason`	`aws.vpc.flow.reject_reason`	🔴

atoulme · 2025-03-24T14:54:59Z

extension/encoding/awslogsencodingextension/config.go

+	formatVPCFlowLog                       = "vpc_flow_log"
+
+	fileFormatPlainText = "plain-text"
+	fileFormatParquet   = "parquet"


gotta document those somewhere eventually. Not in this PR, it's fine.

axw

Looks good overall, but I think we should be translating fields to semantic conventions, and considering additions to SemConv where necessary.

extension/encoding/awslogsencodingextension/config.go

...ncodingextension/internal/unmarshaler/vpc-flow-log/testdata/valid_vpc_flow_log_expected.yaml

...oding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log/vpc_flow_log_unmarshaler.go

axw

Thanks for the updates, nearly there.

extension/encoding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log/const.go

axw · 2025-03-27T01:43:00Z

...oding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log/vpc_flow_log_unmarshaler.go

+	var errGzipReader error
+	gzipReader, ok := v.gzipPool.Get().(*gzip.Reader)
+	if !ok {
+		gzipReader, errGzipReader = gzip.NewReader(bytes.NewReader(content))
+	} else {
+		errGzipReader = gzipReader.Reset(bytes.NewReader(content))
+	}
+	if errGzipReader != nil {
+		if gzipReader != nil {
+			v.gzipPool.Put(gzipReader)
+		}
+		return plog.Logs{}, fmt.Errorf("failed to decompress content: %w", errGzipReader)
+	}
+	defer func() {
+		_ = gzipReader.Close()
+		v.gzipPool.Put(gzipReader)
+	}()


Not for now, but at some point we should look at extracting this into some common code.

I agree. Too many unmarshalers having this :)

...oding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log/vpc_flow_log_unmarshaler.go

axw · 2025-03-27T02:02:58Z

...oding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log/vpc_flow_log_unmarshaler.go

+	case "srcaddr":
+		record.Attributes().PutStr("source.layer.address", value)
+	case "dstaddr":
+		record.Attributes().PutStr("destination.layer.address", value)


The default format only includes srcaddr and dstaddr, and not pkt-srcaddr or pkt-dstaddr. I think in that case we should still set the standard SemConv fields where we can. Also, as mentioned in the other thread, I think if all 4 are available then we should set https://opentelemetry.io/docs/specs/semconv/attributes-registry/network/#network-peer-address depending on the traffic direction.

Unfortunately the default format doesn't include traffic direction either, otherwise I think we could use that to pick the relevant address for source.address or destination.address, and the other for network.peer.address.

The only other option I can think of is: if pkt-srcaddr and pkt-dstaddr are not present, use srcaddr and dstaddr for source.address and destination.address. May be incorrect in the presence of NAT, but we can document this and how to resolve it (i.e. by including pkt-*addr).

The default format only includes srcaddr and dstaddr, and not pkt-srcaddr or pkt-dstaddr.

We aren't settling for the default format. Even a custom format is included in the first line. That's what we are following, so I think we should handle this case already.

I understand we'll handle all the formats, what I meant is that a user may have configured a VPC flow log with just the default format, in which case pkt-* won't be present.

...oding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log/vpc_flow_log_unmarshaler.go

axw · 2025-03-27T07:36:04Z

extension/encoding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log/const.go

+// protocolNames are needed to know the name of the protocol number given by the field
+// protocol in a flow log record.
+// See https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml.
+var protocolNames = [143]string{


I don't think the indices are all correct now. https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml says "nsh" is 145, but we only have 143 entries.

What I was suggesting before is to use this syntax:

var protocolNames = [...]string{ 0: "hopopt", // etc. 255: "reserved", }

Oh, you are absolutely correct 🤔

But then the easier way would be to have a map[int]string, don't you think? Otherwise we need empty values for numbers that do not map to any protocol name.

I think this is a fair tradeoff when there's only 256 entries.

I have added empty entries. I used a unit test to compare that everything was still correct (not committed). If the protocol is (empty), then the attribute protocol name will be empty. I decided to still have the protocol attribute for those cases. What do you think?

That's fine, but I think the syntax I used above is a little safer. You don't have to specify a value for every index that way - the unspecified ones will get an empty string. See https://go.dev/play/p/RKYse-UDfM6

I was unaware this existed. Thanks!!

...oding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log/vpc_flow_log_unmarshaler.go

axw

LGTM, just a few nits. Thanks for your persistence! 😅

...oding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log/vpc_flow_log_unmarshaler.go

…er/vpc-flow-log/vpc_flow_log_unmarshaler.go Co-authored-by: Andrew Wilkins <[email protected]>

constanca-m · 2025-03-28T07:38:59Z

I added a benchmark test yesterday as well. All good there @axw ? It benchmarks unmarshalPlainTextLogs and not UnmarshalLogs, because the gzip took a lot of the statistics to itself.

axw

@constanca-m I'm glad you asked about the benchmark. It looked fine, but I just ran it locally and found the time of each of the sizes was the same, which is surprising - they should be 10x as expensive at each level.

extension/encoding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log/benchmark_test.go

axw

LGTM!

I think the 80MB test is probably unnecessary. It might be useful to have 8K and 8MB in case there's any constant per-file overhead, which we should see amortised by larger files. We can drop the 80MB one later though, it's fine.

constanca-m · 2025-03-28T08:54:22Z

Alright, I don't mind dropping the 80MB file now, if you prefer? Which quantity of logs would fit the benchmark best? @axw

axw · 2025-03-28T08:59:07Z

@constanca-m perhaps just two extremes: 1, and 1000. With 1 we'll see the maximum per-file overhead , and with 1000 it should be drowned out.

constanca-m · 2025-03-28T09:17:18Z

I'm going to add some documentation in an upcoming PR when we get more clarity on all fields. For now it's in the description of the PR, so at least it won't be lost.

Can you merge this PR @atoulme once you have the time please? Thank you!

constanca-m · 2025-03-28T12:46:34Z

@axw I have removed the strings.split like you mentioned, and replaced with strings.Index. It worked much better:

goos: linux
goarch: amd64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/extension/encoding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log
cpu: 11th Gen Intel(R) Core(TM) i7-11800H @ 2.30GHz
                                             │ current.txt │            use-index.txt            │
                                             │   sec/op    │   sec/op     vs base                │
UnmarshalUnmarshalPlainTextLogs/1_log-16       2.265µ ± 2%   2.195µ ± 1%   -3.09% (p=0.000 n=10)
UnmarshalUnmarshalPlainTextLogs/1000_logs-16   660.7µ ± 1%   585.4µ ± 0%  -11.40% (p=0.000 n=10)
geomean                                        38.68µ        35.84µ        -7.34%

                                             │ current.txt  │            use-index.txt             │
                                             │     B/op     │     B/op      vs base                │
UnmarshalUnmarshalPlainTextLogs/1_log-16       5.617Ki ± 0%   5.398Ki ± 0%   -3.89% (p=0.000 n=10)
UnmarshalUnmarshalPlainTextLogs/1000_logs-16   756.4Ki ± 0%   537.6Ki ± 0%  -28.92% (p=0.000 n=10)
geomean                                        65.18Ki        53.87Ki       -17.35%

                                             │ current.txt │           use-index.txt            │
                                             │  allocs/op  │  allocs/op   vs base               │
UnmarshalUnmarshalPlainTextLogs/1_log-16        28.00 ± 0%    27.00 ± 0%  -3.57% (p=0.000 n=10)
UnmarshalUnmarshalPlainTextLogs/1000_logs-16   11.03k ± 0%   10.03k ± 0%  -9.07% (p=0.000 n=10)
geomean                                         555.7         520.3       -6.36%

Thanks for another great suggestion!

…xt-plain-vpc-flow-log

axw

@constanca-m there are several other performance improvements we can make, let's get this merged as it is and defer any more changes to followups please.

...oding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log/vpc_flow_log_unmarshaler.go

constanca-m · 2025-04-01T12:36:00Z

@atoulme Sorry for persistence, can we get this merged?

…ext file format (open-telemetry#38897)  #### Description Add support for VPC flow logs sent to S3 in plain text format.  #### Link to tracking issue Fixes open-telemetry#38896.  #### Testing There are new unit tests added.  #### Documentation Comments in the code and unit tests should be enough. Fields are mapped this way: | Flow log field | Attribute | Available? | Currently supported? | |------------------------------|------------------------------------|----------------|----------------------| | `version` | `aws.vpc.flow.log.version` | 🔴 | | | `account-id` | `cloud.account.id` | 🟢 | | | `interface-id` | `aws.eni.id` | 🔴 | | | `srcaddr` | `source.address` or `network.peer.address` | 🔴 | | | `pkt-srcaddr` | `source.address` | 🟢 | | | `dstaddr` | `destination.address` or `network.peer.address` | 🔴 | | | `pkt-dstaddr` | `destination.address` | 🟢 | | | `srcport` | `source.port` | 🟢 | | | `dstport` | `destination.port` | 🟢 | | | `protocol` | `network.protocol.name` | 🟢 | | | `packets` | `aws.vpc.flow.packets` | 🔴 | | | `bytes` | `aws.vpc.flow.bytes` | 🔴 | | | `start` | `aws.vpc.flow.start` | 🔴 | | | `end` | log timestamp | N/A | | | `action` | `aws.vpc.flow.action` | 🔴 | | | `log-status` | `aws.vpc.flow.status` | 🔴 | | | `vpc-id` | `aws.vpc.id` | 🔴 | | | `subnet-id` | `aws.vpc.subnet.id` | 🔴 | | | `instance-id` | `host.id` | 🔴 | | | `tcp-flags` | `network.tcp.flags` | 🔴 | | | `type` | `network.type` | 🟢 | | | `region` | `cloud.region` | 🟢 | | | `az-id` | `aws.az.id` | 🔴 | | | `sublocation-type` | `aws.sublocation.type` | 🔴 | | | `sublocation-id` | `aws.sublocation.id` | 🔴 | | | `pkt-src-aws-service` | `aws.vpc.flow.source.service` | 🔴 | | | `pkt-dst-aws-service` | `aws.vpc.flow.destination.service` | 🔴 | | | `flow-direction` | `network.io.direction` | 🟢 | | | `traffic-path` | `aws.vpc.flow.traffic_path` | 🔴 | | | `ecs-cluster-arn` | `aws.ecs.cluster.arn` | 🟢 | 🔴 | | `ecs-cluster-name` | `aws.ecs.cluster.name` | 🔴 | 🔴 | | `ecs-container-instance-arn` | `aws.ecs.container.instance.arn` | 🔴 | 🔴 | | `ecs-container-instance-id` | `aws.ecs.container.instance.id` | 🔴 | 🔴 | | `ecs-container-id` | `aws.ecs.container.id` | 🔴 | 🔴 | | `ecs-second-container-id` | `aws.ecs.second.container.arn` | 🔴 | 🔴 | | `ecs-service-name` | `aws.ecs.service.name` | 🔴 | 🔴 | | `ecs-task-definition-arn` | `aws.ecs.task.definition.arn` | 🔴 | 🔴 | | `ecs-task-arn` | `aws.ecs.task.arn` | 🟢 | 🔴 | | `ecs-task-id` | `aws.ecs.task.id` | 🟢 | 🔴 | | `reject-reason` | `aws.vpc.flow.reject_reason` | 🔴 | | --------- Co-authored-by: Andrew Wilkins <[email protected]>

constanca-m added 2 commits March 24, 2025 11:31

Add support for VPC flow logs in plain text file format

b34c57d

Add support for VPC flow logs in plain text file format

7551a5f

constanca-m requested a review from a team as a code owner March 24, 2025 10:35

constanca-m requested a review from mwear March 24, 2025 10:35

github-actions bot assigned mx-psi Mar 24, 2025

github-actions bot added the extension/encoding/awslogsencoding label Mar 24, 2025

github-actions bot requested a review from axw March 24, 2025 10:36

atoulme reviewed Mar 24, 2025

View reviewed changes

atoulme approved these changes Mar 24, 2025

View reviewed changes

atoulme added the waiting-for-code-owners label Mar 24, 2025

github-actions bot mentioned this pull request Mar 25, 2025

Weekly Report: 2025-03-18 - 2025-03-25 #38935

Closed

axw suggested changes Mar 25, 2025

View reviewed changes

constanca-m added 4 commits March 25, 2025 15:27

Address comments

cbc7681

Change fields name, print warning

9361884

add protocol

1105c39

make gotidy

37ccb8c

axw reviewed Mar 27, 2025

View reviewed changes

constanca-m added 2 commits March 27, 2025 08:18

change names, adress rest of comments

ae4f140

fix english

3d2c3fc

axw reviewed Mar 27, 2025

View reviewed changes

constanca-m and others added 4 commits March 27, 2025 09:50

fix address convention, fix protocol list

eea1551

Add benchmarking tests

4f8836e

Merge branch 'main' into text-plain-vpc-flow-log

d873873

Merge branch 'main' into text-plain-vpc-flow-log

650e915

axw approved these changes Mar 28, 2025

View reviewed changes

constanca-m and others added 3 commits March 28, 2025 08:21

Update extension/encoding/awslogsencodingextension/internal/unmarshal…

34a5765

…er/vpc-flow-log/vpc_flow_log_unmarshaler.go Co-authored-by: Andrew Wilkins <[email protected]>

Update extension/encoding/awslogsencodingextension/internal/unmarshal…

34e0048

…er/vpc-flow-log/vpc_flow_log_unmarshaler.go Co-authored-by: Andrew Wilkins <[email protected]>

print warning

162c307

fix protocolNames

92c27c2

axw suggested changes Mar 28, 2025

View reviewed changes

extension/encoding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log/benchmark_test.go Outdated Show resolved Hide resolved

fix gzip bug

0a14086

axw approved these changes Mar 28, 2025

View reviewed changes

change the number of logs on the benchmark

8a961c5

constanca-m and others added 2 commits March 28, 2025 12:24

Merge branch 'main' into text-plain-vpc-flow-log

6e86de0

use strings.index

6eafb67

Merge remote-tracking branch 'origin/text-plain-vpc-flow-log' into te…

301481d

…xt-plain-vpc-flow-log

axw approved these changes Mar 29, 2025

View reviewed changes

...oding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log/vpc_flow_log_unmarshaler.go Show resolved Hide resolved

...oding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log/vpc_flow_log_unmarshaler.go Show resolved Hide resolved

constanca-m mentioned this pull request Mar 29, 2025

[extension/awslogsencoding] Improve performance on text VPC flow logs #39043

Closed

axw mentioned this pull request Mar 31, 2025

Fix semantic conventions for VPC Flow Logs #39051

Closed

github-actions bot mentioned this pull request Apr 1, 2025

Weekly Report: 2025-03-25 - 2025-04-01 #39070

Closed

songy23 merged commit 39ad979 into open-telemetry:main Apr 1, 2025
171 checks passed

github-actions bot added this to the next release milestone Apr 1, 2025

constanca-m deleted the text-plain-vpc-flow-log branch May 14, 2025 22:46

[extension/awslogs_encoding] Add support for VPC flow logs in plain text file format #38897

[extension/awslogs_encoding] Add support for VPC flow logs in plain text file format #38897

Uh oh!

Conversation

constanca-m commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Link to tracking issue

Testing

Documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

axw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

constanca-m Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

axw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

constanca-m commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

axw left a comment

Choose a reason for hiding this comment

Uh oh!

constanca-m commented Mar 28, 2025

Uh oh!

axw commented Mar 28, 2025

constanca-m commented Mar 24, 2025 •

edited

Loading

constanca-m Mar 27, 2025 •

edited

Loading

constanca-m commented Mar 28, 2025 •

edited

Loading

constanca-m commented Mar 28, 2025 •

edited

Loading