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

[Task] Recording Duration displays incorrect value #134

Closed
Tracked by #11
tthvo opened this issue Sep 28, 2022 · 6 comments · Fixed by #135
Closed
Tracked by #11

[Task] Recording Duration displays incorrect value #134

tthvo opened this issue Sep 28, 2022 · 6 comments · Fixed by #135
Labels
bug Something isn't working

Comments

@tthvo
Copy link
Member

tthvo commented Sep 28, 2022

Downloading a continuous recording (with rule "auto_a_rule") while having another 10-second Profiling recording ("something") started. Then, see the summary:

jdk.ActiveRecording {
  startTime = 18:36:03.531
  duration = 346.949 ms
  id = 1
  name = "auto_a_rule"
  destination = N/A
  maxAge = 9223372036854775 s
  flushInterval = 1.000 s
  maxSize = 0 bytes
  recordingStart = 18:36:03.231
  recordingDuration = 9223372036854775 s
  eventThread = "RMI TCP Connection(8)-10.0.2.100" (javaThreadId = 37)
}

jdk.ActiveRecording {
  startTime = 18:36:03.531
  duration = 346.949 ms
  id = 2
  name = "auto_another_rule"
  destination = N/A
  maxAge = 10.000 s
  flushInterval = 1.000 s
  maxSize = 0 bytes
  recordingStart = 18:36:03.614
  recordingDuration = 9223372036854775 s
  eventThread = "RMI TCP Connection(8)-10.0.2.100" (javaThreadId = 37)
}

jdk.ActiveRecording {
  startTime = 18:36:03.531
  duration = 346.949 ms
  id = 5
  name = "something"
  destination = N/A
  maxAge = 9223372036854775 s
  flushInterval = 1.000 s
  maxSize = 0 bytes
  recordingStart = 18:38:44.494
  recordingDuration = 10.000 s
  eventThread = "RMI TCP Connection(8)-10.0.2.100" (javaThreadId = 37)
}

If I load this continuous recording into grafana, Grafana dashboard only picks up the last value, which is incorrect (displayed as 10s) if I load a recording

@tthvo tthvo added the bug Something isn't working label Sep 28, 2022
@tthvo
Copy link
Member Author

tthvo commented Sep 28, 2022

Looks like we need to save the original name of the recording in memory on datasource and check for the name for this specific event? Any better ideas @andrewazores?

@tthvo tthvo moved this to Todo in 2.2.0 Release Sep 28, 2022
@andrewazores
Copy link
Member

That's an option. I think in any case it could end up being misleading for the user if we just present this as a single duration when there may actually have been multiple overlapping recordings running at the same time.

I think a case could be made to enhance this visualization by actually displaying it as a table with N rows and 2 columns. One row for each jdk.ActiveRecording event, the columns being the recording name and duration. There could also be extra columns for the recordingStart, maxAge, etc. Maybe this belongs on its own separate dashboard too, or else at least in a collapsed section on the main dashboard.

@andrewazores
Copy link
Member

Perhaps the single-tile duration value can still be kept there and link over to the table with more details. The single-tile value could be the maximum of all the ActiveRecording recordingDurations, instead of the value from just the last one.

The idea of informing the datasource about the specific name of the recording that the user was trying to view sounds good, but I worry that this will become cumbersome to work with when the source recording is a file from the archives rather than one from active memory. And ideally in the future we have that API I've talked about where a user can create synthetic recordings by concatenating and truncating archive files by timestamp endpoints to create contiguous timesliced views from discrete recording files - and in that case there really isn't a meaningful name to associate to this source recording file which could be correlated to any of the ActiveRecording events, either.

@tthvo
Copy link
Member Author

tthvo commented Sep 28, 2022

Right! That would invalidate the name-saving way anyway.

Perhaps the single-tile duration value can still be kept there and link over to the table with more details. The single-tile value could be the maximum of all the ActiveRecording recordingDurations, instead of the value from just the last one.

Just a question whether it would be a bit surprising to display another duration (max) other than the one user chose to view? Especially, for continous recording, I think we might need to set to Continuous instead of a numeric value.

@andrewazores
Copy link
Member

andrewazores commented Sep 28, 2022

It could be, but it could also be surprising to the user to choose to view a specific recording and then find that the dashboard is displaying data that is out of range (older than expected), or includes event information that "should" not have been present in the recording they requested, because it was captured by another overlapping recording that either had a longer duration or used an event template with more event types enabled.

If the 9223372036854775 can be mapped on the dashboard display side to Continuous or even some numeric value like 0 or -1 to indicate that this wasn't a normal recording length, then that's probably even better.

I suppose the real best single duration to display is the actual time difference between the oldest event start time and the newest event end time in the recording, of any type, not just reading from the ActiveRecording event data.

@tthvo
Copy link
Member Author

tthvo commented Sep 28, 2022

Right! This will make better sense and applicable to the synthetic recording you said. I guess the recording startTime could be changed to oldest event start time instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants