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

Add a Duration Processor implementation #40753

Closed

Conversation

jbaiera
Copy link
Member

@jbaiera jbaiera commented Apr 2, 2019

This processor allows for human readable durations (e.g. 5s) to be converted into a value representing the duration in milliseconds (e.g. 5000). The processor can be configured to convert the duration into a time unit other than milliseconds.

This processor makes use of the TimeValue.parseTimeValue(String, String) method to parse the given field value into an instance of TimeValue before converting it into a number of milliseconds (or other configured time unit).

In order to support some general monitoring use cases, the TimeValue class has been modified to accept fractional time values, with additional guards in place to avoid long overflow/underflow when converting large units.

Update TimeValue to accept fractional values with added protections
against overflow and underflow. Included are tests and documentation.
@jbaiera jbaiera added >enhancement :Core/Infra/Core Core issues without another label :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP labels Apr 2, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@jasontedor
Copy link
Member

In order to support some general monitoring use cases, the TimeValue class has been modified to accept fractional time values, with additional guards in place to avoid long overflow/underflow when converting large units.

It was deliberate that TimeValue does not support fractional time values. Can we discuss this?

@jbaiera
Copy link
Member Author

jbaiera commented Apr 3, 2019

It was deliberate that TimeValue does not support fractional time values. Can we discuss this?

I had a feeling that was the case. One of the main drivers for supporting the fractional time values was that this processor may be used for monitoring Elasticsearch itself, which may occasionally report time values in fractional format. Perhaps it makes sense to move the fractional parsing to the processor only?

@jasontedor
Copy link
Member

That would be my inclination if we absolutely must do this, but I think we should consider changing Elasticsearch so that it doesn't report fractional time values in these APIs.

@jbaiera
Copy link
Member Author

jbaiera commented Apr 3, 2019

relates #39857

@jbaiera
Copy link
Member Author

jbaiera commented Apr 3, 2019

One of the driving factors, for reference: elastic/beats#9603

Looking into the service, the TimeValue's toString method is relied on for logging these events. I wouldn't be surprised if that were the case in countless other places.

Maybe we should have a discussion about breaking out the toString method in TimeValue to explicitly report its time unit, but have a separate prettyPrint()-like method for rendering the human friendly representation?

@jbaiera
Copy link
Member Author

jbaiera commented Feb 20, 2020

Closing this PR for now. There's quite a bit of contention around parsing fractional time values. In order to solve the original problem it would probably be better to remove the logging of fractional time values and simply log the un-formatted time value in places where it might need to be captured for further processing.

@jbaiera jbaiera closed this Feb 20, 2020
@jbaiera jbaiera deleted the feature-ingest-time-duration-processor branch February 20, 2020 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants