Skip to content

[Apache_Tomcat] Ingest pipeline pattern enhancement #13896

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

[Apache_Tomcat] Ingest pipeline pattern enhancement #13896

wants to merge 11 commits into from

Conversation

Linu-Elias
Copy link
Contributor

@Linu-Elias Linu-Elias commented May 13, 2025 •
edited
Loading

Proposed commit message

Added support for %D attribute to get the time taken to process the request, in millis.
Existing Supported Pattern:
Common Log Format :- '%h %l %u %t "%r" %s %b'
Combined Log Format :- '%h %l %u %t "%r" %s %b "%{Referrer}i" "%{User-Agent}i"'
Combined Log Format + X-Forwarded-For header :- '%h %l %u %t "%r" %s %b %A %X %T "%{Referer}i" "%{User-Agent}i" X-Forwarded-For="%{X-Forwarded-For}i"'

New Pattern:
Common Log Format :- '%h %l %u %t "%r" %s %b ms:%D'
Combined Log Format :- '%h %l %u %t "%r" %s %b ms:%D "%{Referrer}i" "%{User-Agent}i"'
Combined Log Format + X-Forwarded-For header :- '%h %l %u %t "%r" %s %b ms:%D %A %X %T "%{Referer}i" "%{User-Agent}i" X-Forwarded-For="%{X-Forwarded-For}i"'

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.
  • I have verified that any added dashboard complies with Kibana's Dashboard good practices

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Screenshots

@Linu-Elias Linu-Elias self-assigned this May 14, 2025
@Linu-Elias
Copy link
Contributor Author

/test

@elastic-vault-github-plugin-prod
Copy link

elastic-vault-github-plugin-prod bot commented May 19, 2025 •
edited
Loading

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@Linu-Elias Linu-Elias force-pushed the apache_add_pattern branch from b482f9e to 66265ac Compare May 19, 2025 14:13
@Linu-Elias Linu-Elias marked this pull request as ready for review May 21, 2025 10:46
@Linu-Elias Linu-Elias requested a review from a team as a code owner May 21, 2025 10:46
@ishleenk17
Copy link
Contributor

@Linu-Elias : As per documentation %T also gives the same time just in seconds..
%D - Time taken to process the request, in millis
%T - Time taken to process the request, in seconds

So are both same ? If yes, do we need both of them in the log format ?

@Linu-Elias
Copy link
Contributor Author

@ishleenk17, I was about to add a comment regarding a gap.
According to the documentation, we do have support for %T. But, after closely looking at the sample logs in the ingest pipeline and the corresponding Grok pattern, it appears that we actually support %F (time taken to commit the response, in milliseconds), which is mapped to apache_tomcat.access.response_time.
Kindly let me know you inputs on this.

@andrewkroh andrewkroh added the Team:Obs-InfraObs Observability Infrastructure Monitoring team [elastic/obs-infraobs-integrations] label May 21, 2025
@ishleenk17
Copy link
Contributor

But, after closely looking at the sample logs in the ingest pipeline and the corresponding Grok pattern, it appears that we actually support %F (time taken to commit the response, in milliseconds), which is mapped to apache_tomcat.access.response_time

Can we confirm on this from the apache tomcat logs and if required do the correction in the current code.
So from what I ubderstand you are mentiioning that we are groking correctly but the value corresponds to a field with different definition.
Lets confirm that and make the change else it would be confusing.
Also please see it should not be a breaking change.

@Linu-Elias
Copy link
Contributor Author

But, after closely looking at the sample logs in the ingest pipeline and the corresponding Grok pattern, it appears that we actually support %F (time taken to commit the response, in milliseconds), which is mapped to apache_tomcat.access.response_time

Can we confirm on this from the apache tomcat logs and if required do the correction in the current code. So from what I ubderstand you are mentiioning that we are groking correctly but the value corresponds to a field with different definition. Lets confirm that and make the change else it would be confusing. Also please see it should not be a breaking change.

I have made the necessary doc changes in this PR for- changed %T to %F since we are grokking for apache_tomcat.access.response_time

ishleenk17
ishleenk17 reviewed May 26, 2025
View reviewed changes
packages/apache_tomcat/data_stream/access/fields/fields.yml Outdated
@@ -23,3 +23,7 @@
type: double
description: Response time of the endpoint.
unit: s
- name: http.request.process_time
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't this under the apache_tomcat.access fields ?

packages/apache_tomcat/data_stream/access/fields/fields.yml Outdated
@@ -23,3 +23,7 @@
type: double
description: Response time of the endpoint.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should change the description of this field as you mentioned here.
#13896 (comment)

@Linu-Elias Linu-Elias force-pushed the apache_add_pattern branch from 08cd592 to a1ce6d5 Compare May 27, 2025 04:50
ishleenk17
ishleenk17 reviewed May 27, 2025
View reviewed changes
packages/apache_tomcat/data_stream/catalina/fields/beats.yml
@@ -22,3 +22,6 @@
- name: vol
type: keyword
description: The serial number of the volume that contains a file. (Windows-only)
- name: log.flags
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this added for ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The build was failing because of missing field
test case failed: one or more errors found in mappings in logs-apache_tomcat.catalina index template: [0] field "log.flags" is undefined: field definition not found

ishleenk17
ishleenk17 approved these changes May 27, 2025
View reviewed changes
Copy link
Contributor

@ishleenk17 ishleenk17 left a comment

Choose a reason for hiding this comment

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

1 last comment.
Otherwise looks good!

@elastic-sonarqube Elastic SonarQube
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

@elasticmachine
Copy link

💚 Build Succeeded

History

cc @Linu-Elias

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ishleenk17 ishleenk17 ishleenk17 approved these changes

Assignees

@Linu-Elias Linu-Elias

Labels
Integration:apache_tomcat Apache Tomcat Team:Obs-InfraObs Observability Infrastructure Monitoring team [elastic/obs-infraobs-integrations]
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Linu-Elias @ishleenk17 @elasticmachine @andrewkroh