-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[extension/azure_encoding] Add processing for AppService, CDN, FrontDoor and Recommendation logs #44919
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
base: main
Are you sure you want to change the base?
[extension/azure_encoding] Add processing for AppService, CDN, FrontDoor and Recommendation logs #44919
Conversation
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
| | `cacheStatus` | `azure.cache_status` | Log Attribute | | ||
| | `errorInfo` | `exception.type` | Log Attribute | | ||
| | `endpoint` | `destination.address` + `destination.port` if it is equal to `hostName` or `network.peer.address` + `network.peer.port` otherwise. If unparsable - `destination.address.original` | Log Attribute | | ||
| | `hostName` | `destination.address` + `destination.port`. If unparsable - `destination.address.original` | Log Attribute | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not
| | `hostName` | `destination.address` + `destination.port`. If unparsable - `destination.address.original` | Log Attribute | | |
| | `hostName` | `host.name` | Log Attribute | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to http.request.header.host as it described in documentation:
The host name in the request from client. If you enable custom domains and have wildcard domain (*.contoso.com), the HostName log field's value is subdomain-from-client-request.contoso.com. If you use the Azure Front Door domain (contoso-123.z01.azurefd.net), the HostName log field's value is contoso-123.z01.azurefd.net
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
thompson-tomo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have attempted to be consistent with network attributes. Key thing is client/server attributes are 1 pair source/destination is another pair and both pairs shouldn't be used together.
| | `requestBytes` | `http.request.size` | Log Attribute | | ||
| | `responseBytes` | `http.response.size` | Log Attribute | | ||
| | `userAgent` | `user_agent.original` | Log Attribute | | ||
| | `clientIp` | `client.address` | Log Attribute | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | `clientIp` | `client.address` | Log Attribute | | |
| | `clientIp` | `source.address` | Log Attribute | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think source is only meant to be used when it is not clear this is a client, so it is best to stick with client here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then we shouldn't have destination.* attributes which was already there. In fact this mapping also already has source.* attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Official Microsoft Azure documentation stated that it's "the IP address of the client that made the request", so it should be client.*
| | `responseBytes` | `http.response.size` | Log Attribute | | ||
| | `userAgent` | `user_agent.original` | Log Attribute | | ||
| | `clientIp` | `client.address` | Log Attribute | | ||
| | `clientPort` | `client.port` | Log Attribute | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | `clientPort` | `client.port` | Log Attribute | | |
| | `clientPort` | `source.port` | Log Attribute | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Official Microsoft Azure documentation stated that it's "The IP port of the client that made the request", so it should be client.*
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
|
|
||
| | Azure "properties" Field | OpenTelemetry | OpenTelemetry Scope | | ||
| |---------------------------|-----------------------------------|---------------------| | ||
| | `clientIP` | `client.address` | Log Attribute | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | `clientIP` | `client.address` | Log Attribute | | |
| | `clientIP` | `source.address` | Log Attribute | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me Firewall rules don't fit into the client/server representation given that firewall rules are usually defined using the notion of source/destination.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Official Microsoft Azure documentation stated that it's "The IP address of the client that made the request", so it should be client.*
| | Azure "properties" Field | OpenTelemetry | OpenTelemetry Scope | | ||
| |---------------------------|-----------------------------------|---------------------| | ||
| | `clientIP` | `client.address` | Log Attribute | | ||
| | `clientPort` | `client.port` | Log Attribute | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | `clientPort` | `client.port` | Log Attribute | | |
| | `clientPort` | `source.port` | Log Attribute | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Official Microsoft Azure documentation stated that it's "The IP port of the client that made the request.", so it should be client.*
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
| | `requestBytes` | `http.request.size` | Log Attribute | | ||
| | `responseBytes` | `http.response.size` | Log Attribute | | ||
| | `userAgent` | `user_agent.original` | Log Attribute | | ||
| | `clientIp` | `client.address` | Log Attribute | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | `clientIp` | `client.address` | Log Attribute | | |
| | `clientIp` | `source.address` | Log Attribute | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Official Microsoft Azure documentation stated that it's "The IP address of the client that made the original request", so it should be client.*
| | `responseBytes` | `http.response.size` | Log Attribute | | ||
| | `userAgent` | `user_agent.original` | Log Attribute | | ||
| | `clientIp` | `client.address` | Log Attribute | | ||
| | `clientPort` | `client.port` | Log Attribute | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | `clientPort` | `client.port` | Log Attribute | | |
| | `clientPort` | `source.port` | Log Attribute | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Official Microsoft Azure documentation stated that it's "The IP port of the client that made the request.", so it should be client.*
extension/encoding/azureencodingextension/internal/unmarshaler/logs/README.md
Outdated
Show resolved
Hide resolved
I don't think that using |
|
So the access logs are a hard one to place and they could be client/server however the existing proposal was already using source & destination namespace hence I consolidated on that. |
.../azureencodingextension/internal/unmarshaler/logs/testdata/appservice/http-log_expected.yaml
Outdated
Show resolved
Hide resolved
| "EventStampType": "Stamp", | ||
| "EventPrimaryStampName": "waws-prod-blu-479", | ||
| "EventStampName": "waws-prod-blu-479", | ||
| "Host": "lw0sdlwk0005XR", | ||
| "EventIpAddress": "10.50.0.34" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional that these details are dropped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field is not described in documentation for this category, so I'm not sure how to correctly map it to OpenTelemetry attributes as the meaning of this value is unclear
.../azureencodingextension/internal/unmarshaler/logs/testdata/azurecdn/access-log_expected.yaml
Outdated
Show resolved
Hide resolved
.../azureencodingextension/internal/unmarshaler/logs/testdata/azurecdn/access-log_expected.yaml
Outdated
Show resolved
Hide resolved
|
I have uploaded all the changes discussed here, no more mixtures of As for
|
| // OpenTelemetry attribute name for Azure HTTP Request Duration, | ||
| // from `durationMs` field in Azure Log Record | ||
| attributeAzureRequestDuration = "azure.request.duration" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is client, you should http.client.request.duration, if it is server, you should use http.server.request.duration. See https://opentelemetry.io/docs/specs/semconv/dotnet/dotnet-http-metrics/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, type of value is conflicting with SemConv definition - SemConv states that it should be Histogram while our value is a single number
…oor and Recommendation logs
c5a64d1 to
5b506db
Compare
|
Rebased and updated tests to fulfill new lint requirements (SemConv attribute names in tests should be a string representation now) |
Description
This is a second part from the set of Azure Resource Log parsing implementations in azure_encoding extension.
This part is bringing this extension functionality in parity with
pkg/translator/azurelogsin terms of supported Azure Resource Log Categories (and even a bit more than azurelogs has).Link to tracking issue
Part of #41725
Testing
Corresponding Unit Test were added.
Documentation
Readme is updated with fields mapping information for supported Azure Resource Log Categories.