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

[Metricbeat] Fix fields not being parsed correctly in postgresql/database #29051

Closed

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Nov 19, 2021

What does this PR do?

Fix postgresql.database.blocks.time.read.ms and postgresql.database.blocks.time.write.ms being captured and reported as a long instead of double thus not being reported at all.

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

@sayden sayden added bug Metricbeat Metricbeat Team:Integrations Label for the Integrations team backport-v8.0.0 Automated backport with mergify labels Nov 19, 2021
@sayden sayden self-assigned this Nov 19, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Nov 19, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Nov 19, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 87 min 57 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

description: >
Time spent reading data file blocks by backends in this database, in
milliseconds.
- name: blocks.time.write.ms
type: long
type: double
Copy link
Member

Choose a reason for hiding this comment

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

Please, double check if this change is problematic in a deployment with both versions of Metricbeat, we had recently some bad experiences with type changes that looked safe.

The kind of test you could do is:

  • Install Metricbeat 7.15 and create a visualization using this field (even if it is zero).
  • Double-check that you have a metricbeat-* index pattern where this field appears.
  • In the same deployment, update to Metricbeat with this fix, and check if there is any problem with the visualization or with the index pattern.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah please double check here 😂 Not sure if it's the same case but I accidentally made a breaking change for changing field type from scaled float to long.

@ruflin
Copy link
Member

ruflin commented Nov 22, 2021

Should this also be backported to 7.16?

@mergify
Copy link
Contributor

mergify bot commented Nov 24, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b bugfix/postgresql/fix-bulk_read_write-values upstream/bugfix/postgresql/fix-bulk_read_write-values
git merge upstream/master
git push upstream bugfix/postgresql/fix-bulk_read_write-values

1 similar comment
@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b bugfix/postgresql/fix-bulk_read_write-values upstream/bugfix/postgresql/fix-bulk_read_write-values
git merge upstream/master
git push upstream bugfix/postgresql/fix-bulk_read_write-values

@sayden sayden added backport-v8.1.0 Automated backport with mergify and removed backport-v8.0.0 Automated backport with mergify labels Feb 10, 2022
@jlind23
Copy link
Collaborator

jlind23 commented Mar 31, 2022

@sayden what should we do with this PR?

@ruflin ruflin removed the backport-v8.1.0 Automated backport with mergify label Apr 1, 2022
@mergify
Copy link
Contributor

mergify bot commented Apr 1, 2022

This pull request does not have a backport label. Could you fix it @sayden? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Apr 1, 2022
@ruflin
Copy link
Member

ruflin commented Apr 1, 2022

I removed the backport labels on this PR to make sure it doesn't get backported to any old branches by accident. We can add backport labels when this gets merged.

@ruflin
Copy link
Member

ruflin commented Apr 1, 2022

For the PR itself, lets get this over the line as it is valuable fix.

@sayden sayden closed this Aug 25, 2022
@sayden sayden deleted the bugfix/postgresql/fix-bulk_read_write-values branch August 25, 2022 12:53
@ishleenk17 ishleenk17 requested a review from a team January 4, 2024 07:07
@ishleenk17
Copy link
Contributor

ishleenk17 commented Jan 4, 2024

@kaiyan-sheng @jsoriano :
This issue was brought up recenlty by a support engineer.
Any reason why this was not merged, if you might remember the history here ?
Looks like a customer is facing an issue due to this.

@jsoriano
Copy link
Member

jsoriano commented Jan 8, 2024

@ishleenk17 looking to the comment history only some testing and the decision to backport was pending, change looks good and valuable for the rest. Not sure why this was closed. Feel free to take it over and complete it.

@PBoff
Copy link

PBoff commented Jan 9, 2024

@jsoriano and @ishleenk17 - Do we have any timelines for this?.

@PBoff
Copy link

PBoff commented Jan 9, 2024

@jsoriano and @ishleenk17 - Do we have any timelines for this?.

@ishleenk17
Copy link
Contributor

@kush-elastic : Could you please share the latest on this ?

@kush-elastic
Copy link
Collaborator

I have started working on this. I will post the results of my findings soon.
There are some scenarios I need to verify for post change as discussed with @ishleenk17.
Thanks

@kush-elastic
Copy link
Collaborator

@PBoff and @ishleenk17,
I have raised PR with changes and verified the scenarios presented before. Let's wait for the PR to be reviewed and merged.
Here's PR Link.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Skip notification from the automated backport with mergify bug Metricbeat Metricbeat Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Metricbeat] [PostgreSQL] module [database] metricset - wrong mapping for blk_read_time and blk_write_time
9 participants