-
Notifications
You must be signed in to change notification settings - Fork 392
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
[IBM WebSphere Application Server] Add WebSphere Application Server Integration Package with JDBC data stream #2922
[IBM WebSphere Application Server] Add WebSphere Application Server Integration Package with JDBC data stream #2922
Conversation
…k with current system test
/test |
🌐 Coverage report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requested some changes
..._application_server/data_stream/jdbc/_dev/test/pipeline/test-jdbc-metrics.json-expected.json
Outdated
Show resolved
Hide resolved
..._application_server/data_stream/jdbc/_dev/test/pipeline/test-jdbc-metrics.json-expected.json
Outdated
Show resolved
Hide resolved
..._application_server/data_stream/jdbc/_dev/test/pipeline/test-jdbc-metrics.json-expected.json
Show resolved
Hide resolved
packages/websphere_application_server/data_stream/jdbc/agent/stream/stream.yml.hbs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the adjustments, @kush-elastic. LGTM!
Please make the changes suggested in this PR (if applicable) to the other IBM WAS PRs as well.
/test |
- rename: | ||
field: prometheus.metrics.was_connectionpool_jdbcOperation_time_seconds_total | ||
target_field: websphere_application_server.jdbc.connection.total.operations_seconds | ||
ignore_missing: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason for having the target field as "websphere_application_server.jdbc.connection.total.operations_seconds" ?
If we have it as websphere_application_server.jdbc.was_connectionpool_jdbcOperation_time_seconds_total and follow the same notion for other fields as well, we can reduce the pipeline oto a simple rename processor as:
rename:
field: prometheus.metrics
target_field: websphere_application_server.jdbc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can also have a looped script to do the renaming,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refer to the reply: #2922 (comment)
- rename: | ||
field: prometheus.metrics.was_connectionpool_jdbcOperation_time_seconds_total | ||
target_field: websphere_application_server.jdbc.connection.total.operations_seconds | ||
ignore_missing: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason for having the target field as "websphere_application_server.jdbc.connection.total.operations_seconds" ?
If we have it as websphere_application_server.jdbc.was_connectionpool_jdbcOperation_time_seconds_total and follow the same notion for other fields as well, we can reduce the pipeline oto a simple rename processor as:
rename:
field: prometheus.metrics
target_field: websphere_application_server.jdbc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elastic has a standard field naming convention: https://www.elastic.co/guide/en/beats/devguide/current/event-conventions.html and we've been following the same standards throughout all the integration. All the fields that we are extracting in this integration also follows the same naming standards. Please refer to the shared doc and let me know if you still think we should change it.
CC: @akshay-saraswat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification.
ignore_missing: true | ||
- script: | ||
description: Drops null/empty values recursively | ||
lang: painless |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the preexisting "Drop "Processor here ? To drop the null fields ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you should refer this for drop processor discussion.
version: '3.4' | ||
services: | ||
websphere_application_server: | ||
image: ibmcom/websphere-traditional:9.0.5.10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a latest Websphere image 9.0.5.11
I think it would be a good idea to use that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I have updated image version. Please check it out.
description: Data stream namespace. | ||
- name: '@timestamp' | ||
type: date | ||
description: Event timestamp. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is always a good practice to have the event.* fields have their values populated in base-fields.yml.
Please refer to this as example:
https://github.com/elastic/integrations/blob/main/packages/prometheus/data_stream/collector/fields/base-fields.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have followed this from integration developer guide.
if you still think we should change this please let us know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would mean that the existing fields in it will never change.
But the newer fields can be added. That's what is followed through all the integrations in Elastic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, Thanks. will update it soon.
Hey @ishleenk17, thanks for the review! We have addressed all the review comments and made the required changes and re-requested review. Could you please take a look at the PR again and let us know if you think we should change anything else before we can have an approval from you over the PR? |
Looks good. Approved! |
What does this PR do?
Checklist
How to test this PR locally
elastic-package test
Related issues
Screenshots