Opened 16 months ago

Closed 9 months ago

#23046 closed enhancement (fixed)

Add sub-interface LogDescriptor.LogLine (and the extension to WebServerAccessLogLine)

Reported by: iwakeh Owned by: iwakeh
Priority: Medium Milestone: metrics-lib 2.2.0
Component: Metrics/Library Version:
Severity: Normal Keywords: metrics-2018
Cc: metrics-team Actual Points:
Parent ID: Points:
Reviewer: karsten Sponsor:

Description

Add sub-interface LogDescriptor.LogLine (and the extension to WebServerAccessLogLine), the first interface will simply provide one method that returns a line from the log.

LogDescriptor will need to be extended to provide all LogLines, which includes sub-interface specific parsing and retrieving the unrecognized lines up to a limit (100 for a start).

Sub-interface WebServerAccessLogLine, which extends LogLine, is more specific for access-logs and provides methods for all fields that contain useful data after the sanitation process, i.e., it doesn't provide all Apache log fields.
(cf. discussion in #22983)

Child Tickets

Change History (22)

comment:1 Changed 16 months ago by iwakeh

Status: newneeds_information

This needs confirmation about

  • the unrecognized lines handling limit.
  • the restriction the api to the fields varying in sanitized log lines

Setting to needs information.

comment:2 Changed 16 months ago by iwakeh

Milestone: CollecTor 1.3.0

comment:3 Changed 16 months ago by iwakeh

Milestone: CollecTor 1.3.0metrics-lib 1.4.0

comment:4 Changed 16 months ago by iwakeh

Milestone: metrics-lib 1.4.0metrics-lib 2.1.0

comment:5 Changed 16 months ago by karsten

Regarding the open questions,

  • we discussed today that limiting unrecognized lines to 3 or 10 would be sufficient, and
  • we didn't conclude on restricting the API to the fields varying in sanitized logs yet. However, after giving this some thought I'd say that we can indeed restrict the API for now and add new methods later if we need them. What we should not do is reject log lines with fields that typically do not vary in sanitized logs. But not providing getters for those fields sounds okay to me.

comment:6 Changed 14 months ago by iwakeh

Milestone: metrics-lib 2.1.0metrics-lib 2.2.0

Move to next milestone in order to accommodate interim release.

comment:7 Changed 14 months ago by karsten

Keywords: metrics-2018 added

comment:8 Changed 14 months ago by karsten

Keywords: metrics-2017 added; metrics-2018 removed

comment:9 Changed 14 months ago by iwakeh

Owner: changed from metrics-team to iwakeh
Status: needs_informationaccepted

The first step is to provide the interface definitions and extensions based on the latest branch in #22983.
Once everyone is happy with these definitions the implementation will follow.

comment:10 Changed 11 months ago by iwakeh

Keywords: metrics-2018 added; metrics-2017 removed

Will be completed in 2018.

comment:11 Changed 9 months ago by iwakeh

Cc: metrics-team added
Status: acceptedneeds_review

Please review this branch based on the current master implementation.

comment:12 Changed 9 months ago by karsten

Reviewer: karsten

Will take a look.

comment:13 Changed 9 months ago by karsten

Status: needs_reviewneeds_revision

Please find some trivial tweaks in commit a4a66b1 of my task-23046 branch. If these look okay, I'll squash and push to master. Setting to needs_revision, but really revision here means looking those trivial tweaks only.

comment:14 Changed 9 months ago by iwakeh

Status: needs_revisionmerge_ready

All space corrections and especially the comments look fine!
(I really write very generic/technical Javadoc while coding at the same time ;-)

Setting to merge ready.

comment:15 Changed 9 months ago by karsten

Resolution: fixed
Status: merge_readyclosed

And merged! Closing. Thanks!

comment:16 Changed 9 months ago by iwakeh

Resolution: fixed
Status: closedreopened

This needs a revision: webstats log files can be large and thus, method

public List<? extends Line> logLines() throws DescriptorParseException;

of the current LogDescriptor interface should be replaced by

public Stream<? extends Line> logLines() throws DescriptorParseException;

comment:17 Changed 9 months ago by iwakeh

Status: reopenedaccepted

comment:18 Changed 9 months ago by iwakeh

Status: acceptedneeds_review

Please review one commit (branch based on master).

comment:19 in reply to:  18 Changed 9 months ago by karsten

Status: needs_reviewneeds_revision

Replying to iwakeh:

Please review one commit (branch based on master).

Two issues:

  • This patch has the same issue with count not being reset as #25317.
  • The Javadocs for new parts in LogDescriptor are missing the @since 2.2.0 part. Also, the Line interface doesn't have a Javadoc comment at all.

comment:20 Changed 9 months ago by iwakeh

Status: needs_revisionneeds_review

Please review the fixup commit.

comment:21 Changed 9 months ago by karsten

Status: needs_reviewmerge_ready

Looks good!

comment:22 Changed 9 months ago by karsten

Resolution: fixed
Status: merge_readyclosed

Merged, closing. Thanks!

Note: See TracTickets for help on using tickets.