search

kieker-monitoring / kieker

Kieker's main repository
Apache License 2.0
70 stars 41 forks source link

[KIEKER-650] Configurabele TimeUnit for Analysis Filters #1097

Open rju opened 2 weeks ago

rju commented 2 weeks ago

JIRA Issue: KIEKER-650 Configurabele TimeUnit for Analysis Filters Original Reporter: Jan Waller

The used TimeUnit for Analysis Filters should be configurable.

see also KIEKER-658 Done , KIEKER-517 Done

Checklist:

rju commented 1 week ago

author Jan Waller -- Mon, 25 Feb 2013 15:00:00 +0100

further improvements in 7cbdb49c5512cc2277b0b3a5e7c832c4c0ca1a79

rju commented 1 week ago

author Jan Waller -- Mon, 25 Feb 2013 15:07:18 +0100

Currently the filters are configurable in the timeunit used for their settings. The timeunit of the records can be configured with a central property of the AnalysisController.

Proposal: A new (additional) value for the timeunit of filters: INHERIT (or "" or something), so it uses the same timeunit as the controller.

rju commented 1 week ago

author Jan Waller -- Mon, 25 Feb 2013 15:08:20 +0100

Replying to [jwa|comment:3]:
> Proposal: A new (additional) value for the timeunit of filters: INHERIT (or "" or something), so it uses the same timeunit as the controller.

Actually, this is the current behaviour, when any value other than a valid TimeUnit is given.

rju commented 1 week ago

author Jan Waller -- Mon, 25 Feb 2013 15:21:35 +0100

Replying to [jwa|comment:4]:
> Replying to [jwa|comment:3]:
> > Proposal: A new (additional) value for the timeunit of filters: INHERIT (or "" or something), so it uses the same timeunit as the controller.
>
> Actually, this is the current behaviour, when any value other than a valid TimeUnit is given.

added warnings, when this happens in 3a5799960da0edee8922b6b07a21624f552fcba1

So the proposal would be a special value with no warning

rju commented 1 week ago

author Jan Waller -- Mon, 25 Feb 2013 17:03:25 +0100

Not yet configurable:

rju commented 1 week ago

author Jan Waller -- Mon, 25 Feb 2013 17:40:07 +0100

Replying to [jwa|comment:6]:
> * kieker.tools.traceAnalysis.filter.traceReconstruction.TraceReconstructionFilter
> * currently using MilliSeconds for configuration and assumes nanos for records
among other things fixed in 8d55625093895a45b5e5ca8dd03c5a893e65d857

the rest should be looked at.

rju commented 1 week ago

author Jan Waller -- Mon, 15 Apr 2013 10:08:37 +0200

well, I guess this one missed its window for 1.7

rju commented 1 week ago

author André van Hoorn -- Wed, 2 Oct 2013 13:10:43 +0200

Replying to [jwa|comment:7]:
> the rest should be looked at.
> * TimestampFilter can stay this way

OK, seems to be using the property already.

> * ResponseTimeDecoration would be better to fix

See no need to fix this:
1. Constructor already has argument TimeUnit executionTimeunit. Method registerExecution(..) properly converts to milliseconds (which is the currently fix time unit used for display KIEKER-1059 Done ).
1. Is only constructed by ResponseTimeNodeDecorator in processMessage method, which has TimeUnit timeunit as argument
1. ResponseTimeNodeDecorator.processMessage(..) only invoked by AbstractDependencyGraphFilter.invokeDecorators and AbstractDependencyGraphFilter uses the IProjectContext.CONFIG_PROPERTY_NAME_RECORDS_TIME_UNIT.

> * ExecutionTrace I guess trivial to fix

In my opinion, it makes no sense to make any assumptions about the time unit within this class. Plugins should (and currently do, e.g., TraceReconstructionFilter) assume IProjectContext.CONFIG_PROPERTY_NAME_RECORDS_TIME_UNIT to be the time unit for the time-related fields in ExecutionTrace.

Suggestion:
1. Add comment to ExecutionTrace that no assumptions made about the time unit
1. Mark long getDurationInNanos() Deprecated (to be removed in 1.9)
1. Create method long getDuration() as replacement for long getDurationInNanos()

> * LoggingTimestampConverter would be better, but does not have to be.

In my opinion there's no need because the JavaDoc is very concrete w.r.t. the expected time unit, e.g., 

   /**
     * Converts a timestamp representing the number of nanoseconds since Jan 1,
     * 1970 UTC into a human-readable datetime string given in the UTC timezone.
         ...
         /*
rju commented 1 week ago

author André van Hoorn -- Wed, 2 Oct 2013 13:20:19 +0200

Replying to [avh|comment:13]:
> Suggestion:
> 1. Add comment to ExecutionTrace that no assumptions made about the time unit
> 1. Mark long getDurationInNanos() Deprecated (to be removed in 1.9)
> 1. Create method long getDuration() as replacement for long getDurationInNanos()
>

Implemented in changeset:8479f9495e0afc56bc16c166bbbab9a89206f064/kieker-git