metricbeat/module/postgresql: Add PostgreSQL Replication metricset #35562

zxcSora · 2023-05-24T14:21:23Z

Enhancement

What does this PR do?

Add metricset for replication to Metricbeat PostgreSQL module.

Why is it important?

It is important monitoring PostgreSQL replication

Checklist

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.

Author's Checklist

[ ]

How to test this PR locally

You need 2 PostgreSQL servers, Primary<->Replica.
Build metricbeat from this PR
Run metricbeat with:

- module: postgresql
  metricsets:
    - replication
  hosts: ["postgres://user:password@localhost:5432/postgres"]

Related issues

Closes Add metricset for replication to Metricbeat Postgresql module #11234

Use cases

Screenshots

Screenshots from real primary production server with 2 replicas.

Logs

mergify · 2023-05-24T14:22:45Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @zxcSora? 🙏.
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

elasticmachine · 2023-05-24T14:26:37Z

❕ Build Aborted

The PR is not allowed to run in the CI yet

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-09-19T15:05:24.770+0000
Duration: 5 min 50 sec

Steps errors

Expand to view the steps failures

`Load a resource file from a library`

Took 0 min 0 sec . View more details here
Description: approval-list/elastic/beats.yml

`Error signal`

Took 0 min 0 sec . View more details here
Description: githubApiCall: The REST API call https://api.github.com/orgs/elastic/members/zxcSora return the message : java.lang.Exception: httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/zxcSora : httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/zxcSora : Code: 404Error: {"message":"User does not exist or is not a member of the organization","documentation_url":"https://docs.github.com/rest/orgs/members#check-organization-membership-for-a-user"}

🤖 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 · 2023-05-25T06:23:20Z

Thanks for the contribution @zxcSora .
We will be sharing our reviews on this soon.

shmsr

Thank you for your contribution.

Left some comments that can easily be fixed. I'll next do a thorough technical review as well, please give me more time to review the same.

I left some comments for parts which are even autogenerated. I think we should file a separate issue to fix the template used for autogenerating these.

Here's one: metricset.go.tmpl where some doc comments for some methods (and functions) can be improved. For example: "Fetch method implements" instead of "Fetch methods implements"

metricbeat/docs/fields.asciidoc

metricbeat/module/postgresql/replication/replication.go

metricbeat/module/postgresql/replication/_meta/fields.yml

metricbeat/docs/fields.asciidoc

shmsr

Thank you for your contribution.

Left some comments that can easily be fixed. I'll next do a thorough technical review as well, please give me more time to review the same.

I left some comments for parts which are even autogenerated. I think we should file a separate issue to fix the template used for autogenerating these.

Here's one: metricset.go.tmpl where some doc comments for some methods (and functions) can be improved. For example: "Fetch method implements" instead of "Fetch methods implements"

zxcSora · 2023-05-31T00:56:31Z

Thank you for your contribution.

Left some comments that can easily be fixed. I'll next do a thorough technical review as well, please give me more time to review the same.

I left some comments for parts which are even autogenerated. I think we should file a separate issue to fix the template used for autogenerating these.

Here's one: metricset.go.tmpl where some doc comments for some methods (and functions) can be improved. For example: "Fetch method implements" instead of "Fetch methods implements"

Thanks for your comments. I commited changes. Waiting for technical review

metricbeat/module/postgresql/replication/data.go

ishleenk17 · 2023-05-31T06:14:32Z

metricbeat/module/postgresql/replication/data.go

+	"write_lsn":     c.Str("write_lsn"),
+	"flush_lsn":     c.Str("flush_lsn"),
+	"replay_lsn":    c.Str("replay_lsn"),
+	"write_lag":     c.Str("write_lag"),


All the lag related fields have postgresql datatype of "Interval", which requires 16 bytes storage.
Here we are storing it as string. Doesn't look to be appropriate?

How do you suggest storing?

@shmsr Maybe you can suggest?

Either we have to use a custom data type or maybe string only. Depends.

Example of a custom type: https://github.com/sanyokbig/pqinterval/blob/v1.1.2/interval.go#L11

https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-INTERVAL-INPUT

I'd suggest doing some more research before finalizing what to use. @zxcSora please let us know what are you going to use and why.

in my opinion, sent_lsn, write_lsn, flush_lsn, replay_lsn shoud be a string, because value of this parameters it's like:

sent_lsn | 3D0/F7574390 write_lsn | 3D0/F7574390 flush_lsn | 3D0/F7574390 replay_lsn | 3D0/F7574390

@shmsr I think that making a custom parser does not make sense, so I slightly changed the query to the table so that I get an integer(uint). Means delay in seconds (ignoring nanoseconds). Check last commit

For lsn fields, string datatype looks fine.

metricbeat/module/postgresql/replication/_meta/fields.yml

metricbeat/module/postgresql/replication/data.go

metricbeat/module/postgresql/replication/replication.go

metricbeat/module/postgresql/replication/data.go

metricbeat/module/postgresql/replication/_meta/fields.yml

metricbeat/docs/fields.asciidoc

shmsr · 2023-06-06T07:29:27Z

Also, please don't mark comments as resolved before the reviewers validate if your changes really fixed them especially if the change request is fixing something complex (not some nitpick). I had to open multiple comments just because the changes didn't actually fix them, and I again asked for a change request. If the changes actually fix the issue mentioned, we'll mark them as resolved. Thank you!

libbeat/common/schema/mapstrstr/mapstrstr.go

Co-authored-by: subham sarkar <[email protected]>

ishleenk17

Thanks for addressing multiple review comments.
Looks good!

zxcSora · 2023-07-14T07:46:22Z

Thanks for addressing multiple review comments. Looks good!

How i can request review from another codeowners ? or it will happen automatically ?

ishleenk17 · 2023-07-14T07:51:09Z

@elastic/elastic-agent-data-plane : Could you please review this change.

mergify · 2023-07-27T14:49:40Z

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

fearful-symmetry · 2023-08-01T16:30:20Z

metricbeat/module/postgresql/replication/replication_integration_test.go

+func getConfig(host string) map[string]interface{} {
+	return map[string]interface{}{
+		"module":     "postgresql",
+		"metricsets": []string{"database"},


Was this meant to be "metricsets": []string{"replication"}, instead? In case that was a copy-paste error, I ran it with the change, still got an error from TestFetch

@fearful-symmetry
i think we can't do tests like this because creating cluster from docker-compose its bad idea (too much bad code)

Hmm, okay. Should we just remove this test, then? Find some smaller unit tests we can add here?

i have no idea what i can find in this situation, do you ?

The idea solution here would be to fix the tests or fix the code.

fearful-symmetry · 2023-08-01T16:31:59Z

metricbeat/module/postgresql/replication/replication_integration_test.go

+	if len(errs) > 0 {
+		t.Fatalf("Expected 0 error, had %d: %v", len(errs), errs)
+	}
+	assert.NotEmpty(t, events)


should be require.NotEmpty(), so we don't just panic on the next line if this is empty.

pierrehilbert · 2023-09-05T05:51:45Z

@zxcSora Did you get time to review @fearful-symmetry's comments?

zxcSora · 2023-09-19T14:40:49Z

@zxcSora Did you get time to review @fearful-symmetry's comments?

Have no idea...

fearful-symmetry · 2023-10-04T20:20:12Z

So, it looks like the integration test still isn't passing:

=== RUN   TestFetch
--no-ansi option is deprecated and will be removed in future versions. Use `--ansi never` instead.
--no-ansi option is deprecated and will be removed in future versions. Use `--ansi never` instead.
--no-ansi option is deprecated and will be removed in future versions. Use `--ansi never` instead.
    replication_integration_test.go:43: 
                Error Trace:    /home/alexk/go/src/github.com/elastic/beats/metricbeat/module/postgresql/replication/replication_integration_test.go:43
                Error:          Should NOT be empty, but was []
                Test:           TestFetch
--- FAIL: TestFetch (1.38s)
=== RUN   TestData
--no-ansi option is deprecated and will be removed in future versions. Use `--ansi never` instead.
--no-ansi option is deprecated and will be removed in future versions. Use `--ansi never` instead.
--no-ansi option is deprecated and will be removed in future versions. Use `--ansi never` instead.
    data_generator.go:72: skip data generation tests
--- SKIP: TestData (1.35s)
FAIL
exit status 1
FAIL    github.com/elastic/beats/v7/metricbeat/module/postgresql/replication    2.743s

Not a postgres expert, but I assume it's because a newly-created db instance doesn't have any stats to report? I wonder if we can somehow just inject some statistics, or if we can't, we might just have to skip the integration tests entirely.

pierrehilbert · 2023-10-25T12:07:35Z

@zxcSora any news?

pierrehilbert · 2023-11-08T12:58:07Z

I'm closing for now as we don't have any news.

zxcSora requested review from a team as code owners May 24, 2023 14:21

zxcSora requested review from faec and leehinman May 24, 2023 14:21

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label May 24, 2023

mergify bot assigned zxcSora May 24, 2023

Add PostgresSQL Replication metricset

25e6aff

shmsr changed the title ~~Add PostgresSQL Replication metricset~~ Add PostgreSQL Replication metricset May 25, 2023

shmsr self-requested a review May 30, 2023 05:45

shmsr requested changes May 30, 2023

View reviewed changes

zxcSora requested a review from shmsr May 31, 2023 04:49

ishleenk17 reviewed May 31, 2023

View reviewed changes

metricbeat/module/postgresql/replication/data.go Outdated Show resolved Hide resolved

ishleenk17 reviewed May 31, 2023

View reviewed changes

metricbeat/module/postgresql/replication/_meta/fields.yml Outdated Show resolved Hide resolved

shmsr requested changes May 31, 2023

View reviewed changes

ishleenk17 reviewed May 31, 2023

View reviewed changes

metricbeat/module/postgresql/replication/_meta/fields.yml Outdated Show resolved Hide resolved

metricbeat/module/postgresql/replication/_meta/fields.yml Outdated Show resolved Hide resolved

metricbeat/module/postgresql/replication/_meta/fields.yml Outdated Show resolved Hide resolved

ishleenk17 reviewed May 31, 2023

View reviewed changes

metricbeat/module/postgresql/replication/_meta/fields.yml Outdated Show resolved Hide resolved

shmsr requested changes May 31, 2023

View reviewed changes

metricbeat/module/postgresql/replication/_meta/fields.yml Outdated Show resolved Hide resolved

metricbeat/docs/fields.asciidoc Outdated Show resolved Hide resolved

zxcSora added 3 commits May 31, 2023 12:27

Fixes after review

a3a716b

impove docs

c098a05

add Uint parse

6571445

shmsr reviewed Jun 6, 2023

View reviewed changes

libbeat/common/schema/mapstrstr/mapstrstr.go Outdated Show resolved Hide resolved

libbeat/common/schema/mapstrstr/mapstrstr.go Outdated Show resolved Hide resolved

shmsr mentioned this pull request Jun 6, 2023

Grammatical mistakes in template *.tmpl files #35696

Closed

zxcSora requested a review from a team as a code owner June 6, 2023 09:55

Update CHANGELOG.next.asciidoc

300b3e3

Co-authored-by: subham sarkar <[email protected]>

zxcSora requested a review from ishleenk17 July 11, 2023 08:21

ishleenk17 approved these changes Jul 14, 2023

View reviewed changes

Merge branch 'main' into main

beec498

zxcSora added 4 commits July 17, 2023 18:40

Merge branch 'main' into main

ae6cc32

Merge branch 'main' into main

eec0bd2

Merge branch 'main' into main

4a84a0f

Merge branch 'main' into main

9d1e267

Merge branch 'main' into main

f33bc75

pierrehilbert added the Team:Elastic-Agent Label for the Agent team label Jul 28, 2023

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jul 28, 2023

pierrehilbert requested a review from fearful-symmetry July 28, 2023 06:07

zxcSora added 4 commits July 28, 2023 11:51

Merge branch 'main' into main

0d1c619

Merge branch 'main' into main

ee7ca86

Merge branch 'main' into main

bf6179b

Merge branch 'main' into main

55f3359

fearful-symmetry requested changes Aug 1, 2023

View reviewed changes

lalit-satapathy mentioned this pull request Aug 3, 2023

Add metricset for replication to Metricbeat Postgresql module #11234

Open

zxcSora and others added 2 commits September 19, 2023 18:14

Merge branch 'main' into main

dd53fa4

fix panic

3adfd57

fix copy-paste misstake

659bb76

pierrehilbert closed this Nov 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

metricbeat/module/postgresql: Add PostgreSQL Replication metricset #35562

metricbeat/module/postgresql: Add PostgreSQL Replication metricset #35562

zxcSora commented May 24, 2023 •

edited

Loading

mergify bot commented May 24, 2023

elasticmachine commented May 24, 2023 •

edited by jenkins-beats-ci bot

Loading

Build stats

`Load a resource file from a library`

`Error signal`

ishleenk17 commented May 25, 2023

shmsr left a comment

shmsr left a comment

zxcSora commented May 31, 2023

ishleenk17 May 31, 2023 •

edited by shmsr

Loading

zxcSora May 31, 2023

zxcSora May 31, 2023

shmsr May 31, 2023

zxcSora Jun 1, 2023 •

edited

Loading

zxcSora Jun 6, 2023 •

edited

Loading

ishleenk17 Jul 3, 2023

shmsr commented Jun 6, 2023

ishleenk17 left a comment •

edited

Loading

zxcSora commented Jul 14, 2023

ishleenk17 commented Jul 14, 2023

mergify bot commented Jul 27, 2023

fearful-symmetry Aug 1, 2023

zxcSora Aug 2, 2023 •

edited

Loading

fearful-symmetry Aug 3, 2023

zxcSora Aug 11, 2023

fearful-symmetry Aug 15, 2023

fearful-symmetry Aug 1, 2023

zxcSora Sep 19, 2023

pierrehilbert commented Sep 5, 2023

zxcSora commented Sep 19, 2023

fearful-symmetry commented Oct 4, 2023 •

edited

Loading

pierrehilbert commented Oct 25, 2023

pierrehilbert commented Nov 8, 2023

metricbeat/module/postgresql: Add PostgreSQL Replication metricset #35562

metricbeat/module/postgresql: Add PostgreSQL Replication metricset #35562

Conversation

zxcSora commented May 24, 2023 • edited Loading

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

mergify bot commented May 24, 2023

elasticmachine commented May 24, 2023 • edited by jenkins-beats-ci bot Loading

❕ Build Aborted

Build stats

Steps errors

Load a resource file from a library

Error signal

🤖 GitHub comments

ishleenk17 commented May 25, 2023

shmsr left a comment

Choose a reason for hiding this comment

shmsr left a comment

Choose a reason for hiding this comment

zxcSora commented May 31, 2023

ishleenk17 May 31, 2023 • edited by shmsr Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zxcSora Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

zxcSora Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmsr commented Jun 6, 2023

ishleenk17 left a comment • edited Loading

Choose a reason for hiding this comment

zxcSora commented Jul 14, 2023

ishleenk17 commented Jul 14, 2023

mergify bot commented Jul 27, 2023

Choose a reason for hiding this comment

zxcSora Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierrehilbert commented Sep 5, 2023

zxcSora commented Sep 19, 2023

fearful-symmetry commented Oct 4, 2023 • edited Loading

pierrehilbert commented Oct 25, 2023

pierrehilbert commented Nov 8, 2023

zxcSora commented May 24, 2023 •

edited

Loading

elasticmachine commented May 24, 2023 •

edited by jenkins-beats-ci bot

Loading

`Load a resource file from a library`

`Error signal`

ishleenk17 May 31, 2023 •

edited by shmsr

Loading

zxcSora Jun 1, 2023 •

edited

Loading

zxcSora Jun 6, 2023 •

edited

Loading

ishleenk17 left a comment •

edited

Loading

zxcSora Aug 2, 2023 •

edited

Loading

fearful-symmetry commented Oct 4, 2023 •

edited

Loading