GEODE-7216 include a timestamp in stack trace logs

[mkevo]

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

• Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

• Has your PR been rebased against the latest commit within the target branch (typically develop)?

• Is your initial contribution a single, squashed commit?

• Does gradlew build run cleanly?

• Have you written or updated unit tests to verify your changes?

• If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

Note:

Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

[jujoramos]

Hey @mkevo, thanks for the contribution!.
Can you please add tests to verify your changes?.

[aaronlindsey]

Did you consider using java.time.* classes instead of java.util.Date? I think the best practice is to use the former unless there is a reason why only java.util.Date will do. Jon Skeet has an interesting article about the difference.

If you do switch to a java.time.* class, you'll have to be careful about time zones. By default java.util.Date uses the system-local time zone which in this case happens to be the same as the time zone in the logs so it works out by happenstance. But if you switch to java.time.Instant (basically the analogue to Date), that class defaults to UTC. You'd have to make sure the string version which is output by the command matches the time zone in the logs.

[mkevo]

Thanks @aaronlindsey! Great article! I will change it.

[aaronlindsey]

Would StackTracesPerMember ever be deserialized from an older version that didn't include the timestamp field? I'm just asking — I don't actually know. Maybe @jujoramos would know. If this could happen, then the timestamp field would be null and the formatter.format(...) line in ExportStackTraceCommand could throw a NPE.

[jujoramos]

@aaronlindsey: good catch, I haven't thought about it but my guess is that it could happen, yes. I'm not a 100% sure, though.

[mkevo]

Do we need to use try/catch on formatter or override toData/fromData in StackTracesPerMember?

[aaronlindsey]

I would do something like the following: Add package private method in ExportStackTraceCommand which handles generating the header message. Then you can write a unit test for this more easily. I don't see an easy way to write an integeration (D-unit) test for this case.

  @VisibleForTesting
  String getHeaderMessage(StackTracesPerMember stackTracesPerMember) {
    String headerMessage = stackTracesPerMember.getMemberNameOrId();

    Instant timestamp = stackTracesPerMember.getTimestamp();
    if (timestamp != null) {
      headerMessage += " at " + formatter.format(timestamp);
    }

    return headerMessage;
  }

The issue with overriding readObject in StackTracesPerMember is that you wouldn't know which value to assign to the timestamp field when the object is deserialized.

[aaronlindsey]

Thanks for addressing my feedback about the possibility of the timestamp field being null when deserializing a StackTracesPerMember. However, I think the test would be more effective if you split it out into a couple of more specific scenarios. For example:

1. When I pass a value of null as the timestamp for StackTracesPerMember, I expect that the headerMessage to be exactly "server".
2. When I pass a specific value like "2000/01/02 03:04:05.006" as the timestamp for StackTracesPerMember, I expect the headerMessage to be exactly "server at 2000/01/02 03:04:05.006".

[mkevo]

Thanks for comments! I added new test with null as the timestamp and modified existing to check if headerMessage has specific format as expected in output of this command.

[aaronlindsey]

Thanks! It looks good.

[mivanac]

Since these changes are approved, I will merge this PR.