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 more fields to log #100

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Adding more fields to log #100

wants to merge 7 commits into from

Conversation

JacksonElia
Copy link
Member

@JacksonElia JacksonElia commented Nov 21, 2024

Why not log even more data? With this PR we need to log:

  • Make it correctly log extension
  • Log Convergence Parameters
  • Log Queue sizes
  • Log the first timestamp/packet which airbrakes.update() runs with

Must be merged after #99

@JacksonElia JacksonElia added enhancement New feature or request data analysis labels Nov 21, 2024
@harshil21
Copy link
Member

I would argue that after #99 we don't need to log queue sizes anymore.

@wlsanderson
Copy link
Contributor

I still think we should tbh. Theres no reason not to. It definitely helps with debugging and finding where performance issues happen, and it's one of the fields where you cant go back in post-processing and find it out then.

@harshil21
Copy link
Member

well in #99, the fetched packets will be no greater than 15 per iteration

@JacksonElia
Copy link
Member Author

I agree with will. If for some reason in the future they do exceed 15, we wouldn't know that easily,

@wlsanderson
Copy link
Contributor

i mean whats one more column on the log file. Gotta use the whole 128gb if we have 128gb

@harshil21
Copy link
Member

harshil21 commented Nov 22, 2024

alright, it could be ever so slightly more overhead but it's probably worth it.

@harshil21
Copy link
Member

Changes:

  • All packets are now in a separate folder
  • DebugPacket is only logged for the first packet processed, so that way we know which is the first packet the batch was, and also saves some space since those fields would be repeated for that batch anyway.

Copy link
Contributor

@wlsanderson wlsanderson left a comment

Choose a reason for hiding this comment

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

I feel like the debug packet is just not an elegant solution to this issue. While I can see the purpose of logging at the start of each set of packets, this seems like it adds a lot more complexity to the issue. Passing the DebugPacket into a LoggedDataPacket just doesn't really sit right with me lol. I think just having these values logged every line (even if they don't update) is a cleaner solution, and arguably easier to do post-processing with.

@harshil21
Copy link
Member

Well we do have a lot of fields to log and the log() method is just getting too crowded. We are probably going to have more fields we would want to log in the future.

If we log these for the entire batch, then we would need another field to know which packet was the first one in the batch.

I guess it's now up to @JacksonElia to break the tie.

@JacksonElia
Copy link
Member Author

I agree with Will here, I don't think I like the DebugDataPacket. Though because we have multiple fields we are logging from the apogee predictor, I would be fine with an apogee_prediction_data_packets. As for the queue sizes, I think they should be logged separately/individually.

After thinking about this a lot though, I think that the cleanest solution could eventually be using **kwargs when logging. Maybe I'll add it eventually.

@harshil21
Copy link
Member

I would be fine with an apogee_prediction_data_packets.

So another data packet class but would only contain things from that process? I can get behind that. How do you want to send the packets from that process to the main process? Do you want to include predicted_apogee in that too? Another Queue?

After thinking about this a lot though, I think that the cleanest solution could eventually be using **kwargs when logging.

Where? Some more details would be nice

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

Successfully merging this pull request may close these issues.

Log the actual servo extension instead of the one set in AirbrakesContext
3 participants