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

Adding network information to logs #173

Merged
merged 3 commits into from Aug 21, 2023

Conversation

LikeTheSalad
Copy link
Contributor

Closes #140

This PR adds net.host.connection.type (and net.host.connection.subtype when available) to all log signals. These attr names arrive at ES as network.connection.type and network.connection.subtype respectively.

@felixbarny
Copy link
Member

I was under the impression that we just wanted to add network information to errors and crashes, not all logs.

@LikeTheSalad
Copy link
Contributor Author

Yes, we did mention that last week, though yesterday we decided to do so for all logs. If I remember correctly I think it was due to the fact that our users can send custom errors by sending logs with a "type=error" attribute (or something similar, I can't remember the exact attr right now), so if we added these extra attrs only to the crashes and errors that we create, then the custom errors would have the same filtering issue.

@felixbarny
Copy link
Member

Oh, I see. When does this attribute visitor run? Could we check if the attributes contain type=error and only add the network info in that case?

@LikeTheSalad
Copy link
Contributor Author

The visitor gets called every time a log is emitted so it is possible to adjust its behaviour later. There are a couple of issues with reading attrs from each log at this stage though. One is that our visitor currently only receives an instance of AttributesBuilder which doesn't allow reading from it, and the other one is that the log as we get it in the processor is of type ReadWriteLogRecord which also doesn't allow to read attrs from itself, although we could call a method it contains to transform it into a LogRecordData which is readable, though that process will happen again automatically when sending the log to the exporter, so I'm concerned about duplicating that work.

It would take more work to exclude some logs from having this info than adding it to all, it's not impossible though, we could do a little refactor and provide the existing attrs along with the AttributesVisitor which could enable us to add this logic, or we can even move the visitor to the exporter where we wouldn't have to repeat the work of converting the logs to a readable format. Though in the end what concerns me about this is that, since the UI has this general filter that always relies on the network type info to be available, how can we tell that a similar issue won't arise in the future if we decide to exclude some logs from having it?

@LikeTheSalad LikeTheSalad merged commit 318686d into main Aug 21, 2023
4 checks passed
APM-Agents (OLD) automation moved this from In Progress to Done Aug 21, 2023
@LikeTheSalad LikeTheSalad deleted the 140-add-network-information-to-logs branch August 21, 2023 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Add network information to logs
3 participants