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

fix: removed unnecessary StartupMessage#time, no NPE when exc in error() is null #15

Merged

Conversation

adietish
Copy link
Contributor

@adietish adietish commented Mar 12, 2021

Different cleanups, the initial idea in this PR was refuted (See comment #15 (comment)).

Keeping the initial idea for documentational/informational purposes:

In tekton ShowLogsAction and FollowLogsAction extend LogBaseAction. When invoked (ex. via context menu) LogBaseAction#actionPerfomed is called.
it would be handy to be able to have telemetry in a instance variable: declare and set the instance variable in ShowLogsAction and FollowLogsAction, use it there. and in the super class LogBaseAction. More handy than to pass telemetry along in a method parameters.
Actions are instantiated once per IDEA session and are reused on each invocation. The telemetry message builder is stateful: you set the parameters and finish by building & sending the message. So there are 2 alternatives:

  • create a new instance in LogBaseAction#actionPerformed
  • clear the builder once the message is sent (so that it can be reused)

@adietish adietish self-assigned this Mar 12, 2021
@adietish adietish requested review from lstocchi and jeffmaury March 12, 2021 21:30
@adietish
Copy link
Contributor Author

adietish commented Mar 15, 2021

to rely on the message builder to mangle it's internal state is a bad idea: clearing the properties is not enough, the "started" timestamp also has to be cleared:
We report a "duration" property to segment. To determine this we need a start/end timestamp. Both can be set explicitly but I try to track it automatically. The message sets the started timestamp when the message is instantiated (TelemetryService.instance().action("follow logs")). "Stopped" is set when the message is sent.
If the message is stored in a instance variable in an IJ Action, the message will have the "started" timestamp of when the IJ Action was instantiated. When the message builder is re-used the "started" timestamp needs to be set explicitly. I could alternatively set it whenever any other property is set. This feels too much error-prone though. Too much magic creates messy, unreliable code.
I thus think that it's far better to require the user programmer to create a new message builder on each Action#actionPerformed.

@adietish adietish changed the title Allow reusing builder instances removed unnecessary StartupMessage#time, no NPE when exc in error() is null Mar 15, 2021
@adietish adietish force-pushed the allow_reusing_builder_instances branch from cfbd503 to 368a4d5 Compare March 15, 2021 20:02
@adietish
Copy link
Contributor Author

@jeffmaury, @lstocchi rebased, please review

@adietish adietish force-pushed the allow_reusing_builder_instances branch from 368a4d5 to b682fe6 Compare March 15, 2021 20:05
Signed-off-by: Andre Dietisheim <adietish@redhat.com>
Signed-off-by: Andre Dietisheim <adietish@redhat.com>
Signed-off-by: Andre Dietisheim <adietish@redhat.com>
@adietish adietish force-pushed the allow_reusing_builder_instances branch 2 times, most recently from c4d985f to 090716b Compare March 15, 2021 21:38
@adietish adietish changed the title removed unnecessary StartupMessage#time, no NPE when exc in error() is null fix: removed unnecessary StartupMessage#time, no NPE when exc in error() is null Mar 15, 2021
Signed-off-by: Andre Dietisheim <adietish@redhat.com>
@adietish adietish force-pushed the allow_reusing_builder_instances branch from 090716b to 851d76e Compare March 15, 2021 21:40
@adietish adietish closed this Mar 16, 2021
@adietish
Copy link
Contributor Author

closed by mistake. Reopening.

@adietish adietish reopened this Mar 16, 2021
Copy link

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

I don't like the renaming because you endup with builder.send() where it should be logically builder.build().send().
So it's ok for me to have non builder beans that returns this in setter methods and that would be easier to understand for novice users.
Unless I missed something

@adietish
Copy link
Contributor Author

I don't like the renaming because you endup with builder.send() where it should be logically builder.build().send().

+1, I became doubtful of the renaming in the meantime but still kept it. Reverting it.

Signed-off-by: Andre Dietisheim <adietish@redhat.com>
@adietish adietish merged commit ecac631 into redhat-developer:main Mar 17, 2021
@adietish adietish deleted the allow_reusing_builder_instances branch March 17, 2021 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants