Skip to content

Conversation

@nikhrom
Copy link

@nikhrom nikhrom commented Sep 29, 2025

Added support for jsonlog log_destination

Now, based on log_destination, the corresponding LogParser will be used.

I also removed the log collector skip if postgres is not a local service but add warn message. This is necessary to be able to mount a directory with postgres logs in a pgscv container, in case pgscv is running in a separate container.

@cherts
Copy link
Owner

cherts commented Sep 29, 2025

Hi, @nikhrom

This is a good feature, but the tests fail with an error:

{"level":"info","time":"2025-09-29T13:24:55Z","message":"starting tail of /tmp/pgscv_postgres_logs_test_sample_1.log from the end"}
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0xde4611]

goroutine 448 [running]:
github.com/cherts/pgscv/internal/collector.tailCollect({0x12141a0, 0xc0000fc7d0}, {0x10e0271, 0x2a}, 0x1, 0xc00011b430, 0xc00016c588)
	/__w/pgscv/pgscv/internal/collector/postgres_logs.go:258 +0x431
github.com/cherts/pgscv/internal/collector.runTailLoop.func1()
	/__w/pgscv/pgscv/internal/collector/postgres_logs.go:204 +0x8f
created by github.com/cherts/pgscv/internal/collector.runTailLoop in goroutine 447
	/__w/pgscv/pgscv/internal/collector/postgres_logs.go:203 +0x2fb
FAIL	github.com/cherts/pgscv/internal/collector	0.633s

@nikhrom
Copy link
Author

nikhrom commented Sep 29, 2025

Ooops

@nikhrom
Copy link
Author

nikhrom commented Sep 29, 2025

Please check it again

@dbulashev
Copy link

dbulashev commented Sep 30, 2025

Hello!
Currently, the queryCurrentLogfile function retrieves the path to the log directory from the database. Please add optional log_directory parameter to the settings, which should take priority over the path provided by the database.

@nikhrom
Copy link
Author

nikhrom commented Sep 30, 2025

Hello
Okay, I'll do it

@nikhrom
Copy link
Author

nikhrom commented Oct 1, 2025

When I started adding log_directory, I realized that I might have misunderstood what was required of me. I added log_directory to the source config, from where I then forward to collector.

Please check

@dbulashev
Copy link

@nikhrom Thank you, I asked for exactly what you delivered.

@nikhrom
Copy link
Author

nikhrom commented Oct 3, 2025

Wait with approve. I had problems with log unmarhsalling while using it at work. I need to figure out what caused the problem

@dbulashev
Copy link

I don't think we'll have any issues with approval, but there are a few blockers for the current release (1.0). If you need to get an image quickly, I recommend cherry-picking it and pushing the MP to the 0.15 branch.

@aleksandrzilber
Copy link

I don't think we'll have any issues with approval, but there are a few blockers for the current release (1.0). If you need to get an image quickly, I recommend cherry-picking it and pushing the MP to the 0.15 branch.

It is worth noting that the marshaling issue can cause excessive load on the disk subsystem and CPU, as the exporter starts reporting problems when parsing JSON logs.

@nikhrom
Copy link
Author

nikhrom commented Oct 3, 2025

if you need to get an image quickly

No, I don't need approval right now. There are logic problems in the current PR that lead to errors in json.Unmarshal when parse json logs. That's why I'm asking you to wait for approval.

@cherts
Copy link
Owner

cherts commented Oct 3, 2025

@nikhrom Hi,

pgSCV shouldn't create any additional, excessive load on the disk or cpu subsystems on the database server.

Please run additional tests and try to fix the issue.

Thank you!

@cherts
Copy link
Owner

cherts commented Oct 3, 2025

@nikhrom and please, fix test

--- FAIL: Test_queryCurrentLogfile (0.01s)
    postgres_logs_test.go:178: 
        	Error Trace:	/__w/pgscv/pgscv/internal/collector/postgres_logs_test.go:178
        	Error:      	Should be true
        	Test:       	Test_queryCurrentLogfile

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants