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][Postgresql][Database] Fix fields not being parsed correctly #37720

Merged

Conversation

kush-elastic
Copy link
Collaborator

@kush-elastic kush-elastic commented Jan 24, 2024

  • Bug

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

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 24, 2024
Copy link
Contributor

mergify bot commented Jan 24, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @kush-elastic? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

Copy link
Contributor

mergify bot commented Jan 24, 2024

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 25301-postgreSql_fix_field_mappings upstream/25301-postgreSql_fix_field_mappings
git merge upstream/main
git push upstream 25301-postgreSql_fix_field_mappings

@elasticmachine
Copy link
Collaborator

❕ Build Aborted

Either there was a build timeout or someone aborted the build.

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

Expand to view the summary

Build stats

  • Duration: 8 min 37 sec

🤖 GitHub comments

Expand to view the 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!)

@elasticmachine
Copy link
Collaborator

💔 Build Failed

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: 51 min 44 sec

Pipeline error 1

This error is likely related to the pipeline itself. Click here
and then you will see the error (either incorrect syntax or an invalid configuration).

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the 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!)

@kush-elastic
Copy link
Collaborator Author

@ishleenk17, I have verified the compatibility of the updated fields for breaking changes, and everything appears to be functioning properly. Here are the steps I followed:

  1. Installed metricbeat 7.15.2.
  2. Configured PostgreSQL.
  3. Ran the Metricbeat setup command to set up the index, template, and dashboards.
  4. Ran Metricbeat to collect data from PostgreSQL and reviewed the dashboard for any issues.
  5. Closed Metricbeat and ran the setup for Metricbeat with the new changes in field mappings.
  6. Ran Metricbeat to collect data and checked the dashboard for issues.
  • No issues were found in the dashboard.
  • The mappings appear to be working correctly due to the addition of a different datastream. A mapping conflict between indexes will not cause an issue.
  • The data view is showing two field types, but this is not causing any issues with data ingestion and the dashboard. Should we consult with someone knowledgeable about this?
Screenshot 2024-01-29 at 12 19 53 PM

@elasticmachine
Copy link
Collaborator

💚 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

  • Start Time: 2024-01-29T07:18:46.062+0000

  • Duration: 50 min 57 sec

Test stats 🧪

Test Results
Failed 0
Passed 4582
Skipped 906
Total 5488

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the 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!)

@kush-elastic kush-elastic marked this pull request as ready for review January 30, 2024 05:26
@kush-elastic kush-elastic requested a review from a team as a code owner January 30, 2024 05:26
@ishleenk17
Copy link
Contributor

  • The mappings appear to be working correctly due to the addition of a different datastream. A mapping conflict between indexes will not cause an issue.

Different datastream?

  • The data view is showing two field types, but this is not causing any issues with data ingestion and the dashboard.

Not sure why it's not showing conflicts, but 2 datatypes means its a conflict. In Integrations, we surely see conflict for such cases. Refer this

CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
@ishleenk17
Copy link
Contributor

Also, once we do this change in beat, corresponding change should be done in Integrations side as well

@kush-elastic
Copy link
Collaborator Author

kush-elastic commented Jan 30, 2024

  • The mappings appear to be working correctly due to the addition of a different datastream. A mapping conflict between indexes will not cause an issue.

Different datastream?

It basically creates new data stream per new version of beat.
For example,
I installed 8.12.0 so it will create metricbeat-8.12.0 datastream.
Now if I upgrade my Metricbeat to 8.13.0 it will create a new datastream which is metricbeat-8.13.0.
Both datastreams will have different indexes and as well as mappings as well.
FYI older beat version does not create a datastream, but a new index per new upgrade so it will work.

  • The data view is showing two field types, but this is not causing any issues with data ingestion and the dashboard.

Not sure why it's not showing conflicts, but 2 datatypes means its a conflict. In Integrations, we surely see conflict for such cases. Refer this

You are correct, I also heard same from @aliabbas-elastic and @niraj-elastic as well.
But maybe it shows a conflict in dataview for the keyword field and not for long to double (Not sure why). I will try to replicate the keyword conflict issue.

@elasticmachine
Copy link
Collaborator

💚 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

  • Start Time: 2024-01-30T08:33:59.315+0000

  • Duration: 52 min 40 sec

Test stats 🧪

Test Results
Failed 0
Passed 4582
Skipped 906
Total 5488

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the 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!)

@ishleenk17
Copy link
Contributor

You are correct, I also heard same from @aliabbas-elastic and @niraj-elastic as well.
But maybe it shows a conflict in dataview for the keyword field and not for long to double (Not sure why). I will try to replicate the keyword conflict issue.

Any further update on this ?

@elasticmachine
Copy link
Collaborator

elasticmachine commented Feb 1, 2024

💚 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

  • Start Time: 2024-03-05T11:12:58.885+0000

  • Duration: 51 min 24 sec

Test stats 🧪

Test Results
Failed 0
Passed 4580
Skipped 908
Total 5488

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the 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!)

@ishleenk17
Copy link
Contributor

Any further update on this ?

@kush-elastic : Let's get this to closure. Any further update on the conflict?

@kush-elastic
Copy link
Collaborator Author

kush-elastic commented Feb 5, 2024

Any further update on this ?

@kush-elastic : Let's get this to closure. Any further update on the conflict?

Hey @ishleenk17, Sorry for the delay.
I tried to replicate the issue, i don't exactly know why but it's working fine for me.
I overridden the current field value type to string(Keyword) but still, i didn't see any errors or warnings there.
Do you think it might based on data sent from metricbeat itself. or maybe i am doing something wrong.

Copy link
Contributor

mergify bot commented Feb 5, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @kush-elastic? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@kush-elastic
Copy link
Collaborator Author

/test

@kush-elastic
Copy link
Collaborator Author

/test

@kush-elastic
Copy link
Collaborator Author

@ishleenk17, can you please help me take a look here? pipeline seems to be failing for one of the tests.
I tried running it multiple times as well.
I also tried to open details in Buildkite but seems to be showing me Page Not Found(I think i dont have access to it.)

@ishleenk17
Copy link
Contributor

/test

@ishleenk17
Copy link
Contributor

@ishleenk17, can you please help me take a look here? pipeline seems to be failing for one of the tests. I tried running it multiple times as well. I also tried to open details in Buildkite but seems to be showing me Page Not Found(I think i dont have access to it.)

The issue looks unrelated. I have retriggered the build

@ishleenk17
Copy link
Contributor

@ishleenk17, can you please help me take a look here? pipeline seems to be failing for one of the tests. I tried running it multiple times as well. I also tried to open details in Buildkite but seems to be showing me Page Not Found(I think i dont have access to it.)

The issue looks unrelated. I have retriggered the build

@kush-elastic : Can you please check with the CI/CD team on this? As I am getting an option to merge the change. So maybe this is a false positive failure.

@kush-elastic
Copy link
Collaborator Author

kush-elastic commented Mar 5, 2024

The issue looks unrelated. I have retriggered the build

@kush-elastic : Can you please check with the CI/CD team on this? As I am getting an option to merge the change. So maybe this is a false positive failure.

Thanks for looking into this.
Sure I will ping CI/CD team to get more details about this.

@ishleenk17
Copy link
Contributor

The issue looks unrelated. I have retriggered the build

@kush-elastic : Can you please check with the CI/CD team on this? As I am getting an option to merge the change. So maybe this is a false positive failure.

Thanks for looking into this. Sure I will ping CI/CD team to get more details about this.

@kush-elastic : I checked with the team who handles CI/CD for beats. This issue can be ignored.

@kush-elastic
Copy link
Collaborator Author

Thanks @ishleenk17,
I will merge this once the current CI is completed (as I updated the branch).

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @kush-elastic

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @kush-elastic

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @kush-elastic

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @kush-elastic

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @kush-elastic

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @kush-elastic

@kush-elastic kush-elastic merged commit a9dfc67 into elastic:main Mar 6, 2024
44 of 45 checks passed
@frconil
Copy link
Contributor

frconil commented Mar 7, 2024

@kush-elastic do you know if this will be backported to 8.13 or 8.12 ?

@kush-elastic kush-elastic added 7.13 candidate backport-v8.13.0 Automated backport with mergify labels Mar 19, 2024
mergify bot pushed a commit that referenced this pull request Mar 19, 2024
…tly (#37720)

* change blk_read_time and blk_write_time from long to double type

* add changelog entry

* remove unnecessary changes

(cherry picked from commit a9dfc67)
@kush-elastic
Copy link
Collaborator Author

@kush-elastic do you know if this will be backported to 8.13 or 8.12 ?

Hey @frconil,
It should be backported to 8.13.
I have added the same. let me know if anything else needs to be done.

kush-elastic added a commit that referenced this pull request Mar 20, 2024
…tly (#37720) (#38411)

* change blk_read_time and blk_write_time from long to double type

* add changelog entry

* remove unnecessary changes

(cherry picked from commit a9dfc67)

Co-authored-by: Kush Rana <89848966+kush-elastic@users.noreply.github.com>
@kush-elastic kush-elastic added the Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team label Mar 20, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.13.0 Automated backport with mergify bugfix Metricbeat Metricbeat Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring 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
6 participants