tweak: Train Crowding Widget Logging #1839
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cmaddox5
requested review from
a team and
hannahpurcell
and removed request for
a team
Coverage of commit
|
Comment on lines
100
to
109
| defp log_crowding_info(prediction, crowding_config) do | ||
| Enum.each( | ||
| prediction.vehicle.carriages, | ||
| fn %Screens.Vehicles.Carriage{} = carriage -> | ||
| Logger.info( | ||
| "[train_crowding car_crowding_info] station_id=#{crowding_config.station_id} direction_id=#{crowding_config.direction_id} car_number=#{carriage.car_number} vehicle_id=#{prediction.vehicle.id} crowding_level=#{carriage.occupancy_status} trip_id=#{prediction.trip.id} prediction_id=#{prediction.id}" | ||
| ) | ||
| end | ||
| ) | ||
| end |
There was a problem hiding this comment.
Instead of separate logging for each car, can we log the array of all car crowding for a vehicle?
There was a problem hiding this comment.
The main reason for splitting out the cars is for easier grouping and comparisons in Splunk tables/dashboards. I worry that keeping all info in the same log will require some complicated log parsing. I guess we could go with one log and, as long as we ensure sorting is consistent, look for any time that the serialized arrays are not equal. Is that what you had in mind?
There was a problem hiding this comment.
Yeah, that's what I had in mind. What is the use case for grouping and comparisons on the car level?
There was a problem hiding this comment.
Aren't we wanting to track changes in crowding at the car level? That's was my motivation going into this task. We are wanting to see how often crowding changes for each trip. Regardless, I think that you're idea should accomplish the same thing with far fewer logs, so I'm gonna try that out.
Coverage of commit
|
Coverage of commit
|
Coverage of commit
|
Coverage of commit
|
Comment on lines
85
to
88
| ScreenData.by_screen_id(screen_id, | ||
| is_real_screen: is_screen, | ||
| screen_id: screen_id, | ||
| triptych_pane: triptych_pane |
There was a problem hiding this comment.
Initially, I was worried that we're passing the triptych_pane for all screen cases, including non-triptychs. I think that means by the time fetch_data is run in ScreenData, all screens except triptych will have the option triptych_pane: nil. Fortunately, right now, the only candidate_generator that pays attention to opts is the triptych one. (But let's say one day we want to use this opts in the candidate_generator for another screen type, that screen will suddenly have this triptych_pane option out of the blue.) I'm thinking it might be better if this option is passed conditionally? I dunno, does that make sense?
There was a problem hiding this comment.
I think one of the advantages to using an opts list is that they can be arbitrary in length and a function doing a lookup in opts only needs to look for what they know they need. Similar to params in this action. This action is used by all screen types but references very specific values only needed by certain screen types. Because a lookup of key that doesn't exist is safe for these lists, we just need to do some nil checks before using them.
I definitely see where you are coming from though. My biggest hesitation with conditionally adding it is that it will make the code here a little more complex for no drastic benefit. What do you think?
There was a problem hiding this comment.
Ok, I can see that. Works for me!
Coverage of commit
|
| evergreen_content_instances_fn \\ &Widgets.Evergreen.evergreen_content_instances/1, | ||
| local_evergreen_set_instances_fn \\ &Widgets.LocalEvergreenSet.local_evergreen_set_instances/1 | ||
| ) do | ||
| [ | ||
| fn -> crowding_widget_instances_fn.(config) end, | ||
| fn -> crowding_widget_instances_fn.(config, opts) end, |
There was a problem hiding this comment.
From our huddle conversation, I suggested this so the train_crowding widget only has the stuff it needs
Suggested change
| fn -> crowding_widget_instances_fn.(config, opts) end, | |
| fn -> crowding_widget_instances_fn.(config, opts[:logging_options]) end, |
There was a problem hiding this comment.
And then the associated changes are needed in the train_crowding candidate_generator
There was a problem hiding this comment.
Ahh I see. I misunderstood. Will make that change now.
Coverage of commit
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Asana task: Add logging to triptych app - pt 1
Added a log for each car so we can make some Splunk charts. The biggest change is a new config for the
Carriageobjects in the API. I wanted to make sure we tracked thecar_numberfor the logs. Once merged, I will work on some Splunk charts/tables.