Skip to content

Various fixes for issue #8 - #9

Merged
bnkamalesh merged 13 commits into
naughtygopher:masterfrom
ph4n70m-nuk3r:changes
Jul 22, 2025
Merged

Various fixes for issue #8#9
bnkamalesh merged 13 commits into
naughtygopher:masterfrom
ph4n70m-nuk3r:changes

Conversation

@ph4n70m-nuk3r

@ph4n70m-nuk3r ph4n70m-nuk3r commented Jul 12, 2025

Copy link
Copy Markdown
Contributor

Partially fixes #8

Comment thread internal/pkg/apm/prometheus.go Outdated
Comment thread internal/pkg/apm/prometheus.go Outdated
Comment thread internal/pkg/sysignals/sysignals.go Outdated
@ph4n70m-nuk3r

Copy link
Copy Markdown
Contributor Author

Also I don't have Go support in the non-commercial IDE I'm using, which is making this a pain to work on:

image

@ph4n70m-nuk3r

ph4n70m-nuk3r commented Jul 19, 2025

Copy link
Copy Markdown
Contributor Author

Here, that is working, if you want to change the use of fmt.Printf(...) to use your own logger implementation, please feel free to do so.
I think the main priority here should be correct initialisation and shutdown of metrics when the app is not running in 'local' mode, and handling the case where no collectorURL is configured for the tracer outside of 'local' mode.

Comment thread internal/pkg/apm/apm.go Outdated
Comment thread internal/pkg/apm/apm.go Outdated
Comment thread internal/pkg/apm/prometheus.go Outdated
Comment thread main.go Outdated
Comment thread internal/pkg/apm/prometheus.go Outdated
@ph4n70m-nuk3r

Copy link
Copy Markdown
Contributor Author

I think I've addressed most of your concerns.

@ph4n70m-nuk3r

Copy link
Copy Markdown
Contributor Author

I think that's now everything as you requested.

Comment thread internal/pkg/apm/apm.go
bnkamalesh
bnkamalesh previously approved these changes Jul 21, 2025

@bnkamalesh bnkamalesh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@bnkamalesh
bnkamalesh enabled auto-merge (squash) July 21, 2025 07:25
@ph4n70m-nuk3r

Copy link
Copy Markdown
Contributor Author
Screenshot_20250721-082614

@ph4n70m-nuk3r

ph4n70m-nuk3r commented Jul 21, 2025

Copy link
Copy Markdown
Contributor Author

I added this (used fmt to test in case logging subsystem was inactive at that point):
Screenshot From 2025-07-21 09-29-25

But after sending SIGTERM I didn't see the output.
Screenshot From 2025-07-21 09-33-17
Screenshot From 2025-07-21 09-30-11

Seems a bit fishy, but for the time being I suppose it can just be:

defer func() {
	_ := healthResp.Shutdown(ctx)
}()

auto-merge was automatically disabled July 21, 2025 08:43

Head branch was pushed to by a user without write access

@bnkamalesh
bnkamalesh enabled auto-merge (squash) July 22, 2025 07:34

@bnkamalesh bnkamalesh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@bnkamalesh
bnkamalesh merged commit 5ad9cb6 into naughtygopher:master Jul 22, 2025
2 checks passed
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.

Numerous issues on master branch.

2 participants