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

Add charts for postgres #3400

Merged
merged 1 commit into from Feb 19, 2018

Conversation

Projects
None yet
4 participants
@anayrat
Copy link
Contributor

anayrat commented Feb 7, 2018

Hello,

This PR add several charts for PostgreSQL:

  • : Rename chart BGWRITER to CHECKPOINTER because it is checkpointer process which triggers checkpoints (https://www.postgresql.org/docs/current/static/wal-configuration.html).
  • : Rename "writer_scheduled" to "checkpoint_scheduled", same for writer_requested.
  • : New BGWRITER chart according to https://www.postgresql.org/docs/10/static/monitoring-stats.html#PG-STAT-BGWRITER-VIEW
  • : New chart for autovacuum, it counts workers and their activity VACUUM, VACUUM ANALYZE, ANALYZE, VACUUM FREEZE
  • : Replication delta. Maybe we should put chart below, I did not find how to change the position (python beginner inside)?
  • : Wal chart renamed to archive wal
  • : New chart wal files : number of WAL files, current number of written WAL, current number of recycled WAL
  • : Add temps files stats per database

Heavily inspired by https://github.com/OPMDG/check_pgactivity

Here the result:

BGWRITER

selection_020

Autovacuum

selection_021

Replication delta

selection_022

Wal

selection_023

Temp files

selection_024

Regards,

@anayrat anayrat changed the title Add background writer metrics postgres - Add background writer metrics Feb 7, 2018

@anayrat anayrat changed the title postgres - Add background writer metrics postgres - Add background writer chart Feb 7, 2018

@anayrat

This comment has been minimized.

Copy link
Contributor

anayrat commented Feb 7, 2018

FYI, I plan to send other PR to add charts. Mainly inspired by https://github.com/OPMDG/check_pgactivity

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Feb 7, 2018

@facetoe please have a look

@anayrat

This comment has been minimized.

Copy link
Contributor

anayrat commented Feb 8, 2018

Would you prefer I send one PR per chart or one PR with several charts ?

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Feb 8, 2018

one PR with several charts

^^

@anayrat anayrat changed the title postgres - Add background writer chart WIP - postgres - Add background writer chart Feb 8, 2018

@anayrat anayrat changed the title WIP - postgres - Add background writer chart WIP - postgres - Add charts Feb 8, 2018

@anayrat anayrat changed the title WIP - postgres - Add charts WIP - Add charts for postgres Feb 8, 2018

@facetoe

This comment has been minimized.

Copy link
Contributor

facetoe commented Feb 9, 2018

Yep, changing the name makes sense. And adding those additional charts would be awesome.

['brin_summarize', 'brin_summarize', 'absolute']
]},
'secondary_delta': {
'options': [None, 'Secondary delta', 'Kbytes', 'replication delta', 'postgres.secondary_delta', 'line'],

This comment has been minimized.

@ilyam8

ilyam8 Feb 10, 2018

Member

Netdata can auto-scale these original units: kilobits/s, kilobytes/s, KB/s, KB, MB, and GB

Kbytes => KB

@anayrat anayrat changed the title WIP - Add charts for postgres Add charts for postgres Feb 10, 2018

@anayrat

This comment has been minimized.

Copy link
Contributor

anayrat commented Feb 10, 2018

I think, I have finished. This PR is ready to be reviewed 😄

['sent_delta', 'sent_delta', 'absolute', 1, 1024],
['write_delta', 'write_delta', 'absolute', 1, 1024],
['flush_delta', 'flush_delta', 'absolute', 1, 1024],
['replay_delta', 'replay_delta', 'absolute', 1, 1024]

This comment has been minimized.

@ilyam8

ilyam8 Feb 11, 2018

Member

if delta is needed in dimension i suggest to use space instead of underscore. It looks better on the dashboard.

 ['replay_delta', 'replay delta', 'absolute', 1, 1024]

Same true for all new dimensions with underscore.

Another problem: netdata applies interpolation to find the exact value at second-boundaries only for incremental values. /s is misleading for absolute (there is no guarantee that the module runs once a second).

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Feb 11, 2018

gj @anayrat 👍

in general all the changes looks ok to me, but i am not a postrgresql user.
@facetoe can you look one more time? what do you think?

@anayrat

This comment has been minimized.

Copy link
Contributor

anayrat commented Feb 11, 2018

Thanks @l2isbad . I fixed label and unit.

['vacuum', 'vacuum', 'absolute'],
['vacuum_analyze', 'vacuum_analyze', 'absolute'],
['vacuum_freeze', 'vacuum_freeze', 'absolute'],
['brin_summarize', 'brin_summarize', 'absolute']

This comment has been minimized.

@ilyam8

ilyam8 Feb 11, 2018

Member

Are you sure all that stuff should be in one chart?

autovacuum_max_workers (integer)

Specifies the maximum number of autovacuum processes (other than the autovacuum launcher) that may be running at any one time. The default is three. This parameter can only be set at server start.

why do we need to chart it?

This comment has been minimized.

@anayrat

anayrat Feb 11, 2018

Contributor

Are you sure all that stuff should be in one chart?

Yes, it shows us autovacuum activity. Then, it give information how to tune autovacuum and other settings...

(autovacuum_max_workers) why do we need to chart it?

You are right, it is not necessary to chart it. Otherwise we should also chart max_connections. I will remove it.

I will also remove "_" in autovacuum labels.

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Feb 11, 2018

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Feb 11, 2018

just found

Replication delta. Maybe we should put chart below, I did not find how to change the position (python beginner inside)

@anayrat
please post the dashboard postgress menu

@anayrat

This comment has been minimized.

Copy link
Contributor

anayrat commented Feb 11, 2018

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Feb 11, 2018

Maybe we should put chart below

what position do you want?

@anayrat

This comment has been minimized.

Copy link
Contributor

anayrat commented Feb 11, 2018

Maybe after all "db *" charts?

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Feb 11, 2018

this shoud work
https://github.com/anayrat/netdata/blob/postgres/python.d/postgres.chart.py#L552

change to

position = order.index('database_size')
order.insert(position, chart_name)
@anayrat

This comment has been minimized.

Copy link
Contributor

anayrat commented Feb 11, 2018

Thanks @l2isbad, it works as expected!

@ktsaou

This comment has been minimized.

Copy link
Member

ktsaou commented Feb 11, 2018

this is nice! let me know when it is ready to merge.

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Feb 12, 2018

lets wait for @facetoe
if he agrees with all changes we can merge it

@facetoe

This comment has been minimized.

Copy link
Contributor

facetoe commented Feb 12, 2018

Looks good to me. Nice work @anayrat!

@ilyam8

ilyam8 approved these changes Feb 12, 2018

@anayrat

This comment has been minimized.

Copy link
Contributor

anayrat commented Feb 13, 2018

FYI, I plan to split bgwriter chart in more charts. In order to separate checkpoint, bgwriter, backend and read/write activity.

@ktsaou

This comment has been minimized.

Copy link
Member

ktsaou commented Feb 13, 2018

nice! so @anayrat merge now or wait for more changes?
since this PR is approved, it is your decision...

@anayrat

This comment has been minimized.

Copy link
Contributor

anayrat commented Feb 14, 2018

It seems finished for me. Here a snapshot.
postgres.zip

And a screenshot :
netdata-postgres

},
'postgres.standby_delta': {
info: 'Streaming replication delta.<ul>' +
'<li><strong>sent_delta:</strong> rReplication delta sent to standby.</li>' +

This comment has been minimized.

@ilyam8

ilyam8 Feb 14, 2018

Member

a typo? rReplication

This comment has been minimized.

@anayrat

anayrat Feb 14, 2018

Contributor

I fixed it. Thanks!

@anayrat anayrat force-pushed the anayrat:postgres branch from c33a40f to 3bb867c Feb 14, 2018

@ktsaou

This comment has been minimized.

Copy link
Member

ktsaou commented Feb 14, 2018

nice!

I saw your snapshot and the only thing that bothers me is this:

image

Temps should be Temp, but I think files and size should not be on the same chart. These are 2 charts, one counting files and other the size of the files. Generally each chart should include dimensions with homogeneous units.

Can you split it in 2 charts?

@anayrat anayrat force-pushed the anayrat:postgres branch 2 times, most recently from 1a2feec to 284a734 Feb 14, 2018

@anayrat

This comment has been minimized.

Copy link
Contributor

anayrat commented Feb 14, 2018

@ktsaou I tried to split temp chart, but I don't understand why "Disk blocks" and "Current connection" are between Temp charts?
temp

@ktsaou

This comment has been minimized.

Copy link
Member

ktsaou commented Feb 14, 2018

I don't understand why "Disk blocks" and "Current connection" are between Temp charts?

I think the order they are placed in the code, defines that. @l2isbad can you help?

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Feb 14, 2018

Yes. Every chart has priority (relative priority of the charts as rendered on the web page, lower numbers make the charts appear before the ones with higher numbers)

Charts are created (created = added to charts) in create() by charts.add_chart()

Default charts priority is 60 000
First chart in order has 60 000, second 60 001 and so on.

@anayrat

This comment has been minimized.

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Feb 15, 2018

@anayrat
have a look

i install your branch to debug the problem and... (clean install on debian 8)

2018-02-15 04:19:06: python.d ERROR: postgres: socket: update() unhandled exception: function pg_current_wal_lsn() does not exist
LINE 3:   pg_wal_lsn_diff(pg_current_wal_lsn() , sent_lsn) AS sent_d...
                          ^
HINT:  No function matches the given name and argument types. You might need to add explicit type casts.

2018-02-15 04:19:06: python.d DEBUG: postgres: socket: update => [FAILED] (elapsed time: -, retries left: 59)
@anayrat

This comment has been minimized.

Copy link
Contributor

anayrat commented Feb 15, 2018

Oh right, view definition has changed in Postgres 10. I should do the same trick as https://github.com/anayrat/netdata/blob/284a734f318b015bf631c1e4f26d63bbb9c5078f/python.d/postgres.chart.py#L439

@anayrat anayrat force-pushed the anayrat:postgres branch from 284a734 to 23bb9ad Feb 15, 2018

@anayrat

This comment has been minimized.

Copy link
Contributor

anayrat commented Feb 15, 2018

@l2isbad Can you try again? I made some changes to handle differences between previous versions and Postgres 10.

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Feb 16, 2018

2018-02-15 19:23:15: python.d ERROR: postgres: socket: update() unhandled exception: function pg_stat_file(text, boolean) does not exist
LINE 14:   ORDER BY (pg_stat_file('pg_xlog/'||name,true)).modificatio...
                     ^
HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
@anayrat

This comment has been minimized.

Copy link
Contributor

anayrat commented Feb 16, 2018

@l2isbad sorry :(. I will try with all Postgres supported version (9.3 -> 10).

@anayrat anayrat force-pushed the anayrat:postgres branch from 23bb9ad to 1881035 Feb 16, 2018

@anayrat

This comment has been minimized.

Copy link
Contributor

anayrat commented Feb 16, 2018

@l2isbad I tried with these Postgres versions:

  • 9.3
  • 9.4
  • 9.5
  • 9.6
  • 10

I also changed postgres.conf. Thanks again for your review.

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Feb 16, 2018

@anayrat
now it works 👍

about the order

  1. please add db_stat_connections to ORDER
  2. change
    https://github.com/anayrat/netdata/blob/postgres/python.d/postgres.chart.py#L455
    to
for chart_name in [name for name in self.order if name.startswith('db_stat')]:
  1. actual order is reverse order from (db_stat_ charts, because of insert(0, chart))
ORDER = ['db_stat_transactions', 'db_stat_blks', 'db_stat_tuple_returned', 'db_stat_tuple_write', 'db_stat_temp_bytes', 'db_stat_temp_files',

db_stat_temp_files first, db_stat_temp_bytes' second and so on

@anayrat anayrat force-pushed the anayrat:postgres branch from 1881035 to e57a2f1 Feb 16, 2018

Add postgresql charts
* Add temp files chart
* Add WAL written chart
* Rename previous bgwriter char to checkpointer
* Add several bgwriter charts
* Add autovacuum chart
* Add replication chart
@anayrat

This comment has been minimized.

Copy link
Contributor

anayrat commented Feb 16, 2018

Good catch for db_stat_connections, it was missing on master! Thanks for the advice for order. It works now.

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Feb 16, 2018

are you done @anayrat ?

@anayrat

This comment has been minimized.

Copy link
Contributor

anayrat commented Feb 16, 2018

Yes, I pushed last changes for order and db_stat_connections

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Feb 16, 2018

@anayrat thx 😄

@ktsaou ready

@ktsaou ktsaou merged commit 04e95a0 into netdata:master Feb 19, 2018

3 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ktsaou

This comment has been minimized.

Copy link
Member

ktsaou commented Feb 19, 2018

merged! great work!

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