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

added prometheus service to docker-compose #1113

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

vaibhavjayaraman
Copy link

Adding Prometheus service to docker-compose

I created a prometheus.yml file and created a prometheus service in docker-compose.yml.

Fixes #970

@vaibhavjayaraman vaibhavjayaraman requested a review from a team as a code owner March 8, 2019 10:23
ports:
- 9090:9090
volumes:
- ./prometheus.yml:/etc/prometheus/prometheus.yml
Copy link
Author

Choose a reason for hiding this comment

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

Is this the best place to put the yaml file in the container? Also, where should i put prometheus.yml in the source code?

Copy link
Member

Choose a reason for hiding this comment

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

@vaibhavjayaraman I put answers below

Is this the best place to put the yaml file in the container?

inside the container, this seems like the canonical place to put the yaml file, so 👍

Also, where should i put prometheus.yml in the source code?

Check out my answer under https://github.com/gomods/athens/pull/1113/files#r263734261

- ./prometheus.yml:/etc/prometheus/prometheus.yml
command:
- '--config.file=/etc/prometheus/prometheus.yml'
- '--storage.tsdb.path=/prometheus'
Copy link
Author

@vaibhavjayaraman vaibhavjayaraman Mar 8, 2019

Choose a reason for hiding this comment

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

Should I create another volume for storage and what should I change the storage location in the container?

Copy link
Member

Choose a reason for hiding this comment

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

yes, I think you should create another volume for storage in here

@codecov-io
Copy link

codecov-io commented Mar 8, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@b5a91e8). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #1113   +/-   ##
========================================
  Coverage          ?   53.1%           
========================================
  Files             ?      80           
  Lines             ?    2785           
  Branches          ?       0           
========================================
  Hits              ?    1479           
  Misses            ?    1178           
  Partials          ?     128

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5a91e8...0d29b71. Read the comment docs.

- targets: ['localhost:9090']
- job_name: 'testunit'
static_configs:
- targets: ['testunit']
Copy link
Author

Choose a reason for hiding this comment

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

When running docker-compose up (and looking at the bash files) it seems that testunit and teste2e get destroyed once they run. Should Prometheus be monitoring these?

Copy link
Member

Choose a reason for hiding this comment

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

@vaibhavjayaraman I don't think so unless you're adding tests for Athens to spit out metrics?

Copy link
Author

Choose a reason for hiding this comment

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

Can you clarify this question for me? Right now I interpret the question to mean that we should include tests that check if the prometheus exporter is working properly by checking if it is spitting out the metrics that we want.

Copy link

Choose a reason for hiding this comment

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

If you wish to add tests for prometheus you are welcome to do so, but if not then you do not need to have prometheus monitor the end to end tests

- targets: ['teste2e']
- job_name: 'athens-proxy'
static_configs:
- targets: ['athens-proxy:3000']
Copy link
Author

Choose a reason for hiding this comment

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

a persistent athens-proxy service doesn't seem to be part of the docker-compose itself so I wasn't sure I was supposed to make Prometheus monitor it. But I made Prometheus be able to query athens-proxy by running athens-proxy in the same docker network (athens_default) . I am not sure if I should do this.

Copy link
Member

Choose a reason for hiding this comment

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

I made Prometheus be able to query athens-proxy by running athens-proxy in the same docker network (athens_default)

@vaibhavjayaraman I think that's a great start! We can add more tests to look for specific metrics as we go 😄

@@ -0,0 +1,18 @@
global:
Copy link
Author

Choose a reason for hiding this comment

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

Where should I put this file in the source code?

Copy link
Member

Choose a reason for hiding this comment

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

@vaibhavjayaraman can you create a new top-level folder called assets/, and put it in that folder?

Copy link
Member

@arschles arschles left a comment

Choose a reason for hiding this comment

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

@vaibhavjayaraman thank you so much for this! 😄

I answered as many questions as I could below

🎉

@@ -0,0 +1,18 @@
global:
Copy link
Member

Choose a reason for hiding this comment

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

@vaibhavjayaraman can you create a new top-level folder called assets/, and put it in that folder?

ports:
- 9090:9090
volumes:
- ./prometheus.yml:/etc/prometheus/prometheus.yml
Copy link
Member

Choose a reason for hiding this comment

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

@vaibhavjayaraman I put answers below

Is this the best place to put the yaml file in the container?

inside the container, this seems like the canonical place to put the yaml file, so 👍

Also, where should i put prometheus.yml in the source code?

Check out my answer under https://github.com/gomods/athens/pull/1113/files#r263734261

- ./prometheus.yml:/etc/prometheus/prometheus.yml
command:
- '--config.file=/etc/prometheus/prometheus.yml'
- '--storage.tsdb.path=/prometheus'
Copy link
Member

Choose a reason for hiding this comment

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

yes, I think you should create another volume for storage in here

- targets: ['localhost:9090']
- job_name: 'testunit'
static_configs:
- targets: ['testunit']
Copy link
Member

Choose a reason for hiding this comment

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

@vaibhavjayaraman I don't think so unless you're adding tests for Athens to spit out metrics?

- targets: ['teste2e']
- job_name: 'athens-proxy'
static_configs:
- targets: ['athens-proxy:3000']
Copy link
Member

Choose a reason for hiding this comment

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

I made Prometheus be able to query athens-proxy by running athens-proxy in the same docker network (athens_default)

@vaibhavjayaraman I think that's a great start! We can add more tests to look for specific metrics as we go 😄

@arschles
Copy link
Member

arschles commented Apr 8, 2019

@vaibhavjayaraman just wanted to check in if you're still wanting to work on this? If not, that's totally fine and we can go finish it up

@vaibhavjayaraman
Copy link
Author

About a week ago, I pushed up another commit (0cfc219) with the changes.

@arschles
Copy link
Member

arschles commented Apr 8, 2019

Sounds good. Do you want to do this? Otherwise we can do it in a follow-up. It's your choice.

@vaibhavjayaraman
Copy link
Author

Sure. I can do it, but can you clarify what you mean?

@marpio
Copy link
Member

marpio commented May 7, 2019

@arschles can you answer vaibhavjayaraman's question?

@arschles
Copy link
Member

arschles commented Jul 8, 2019

@vaibhavjayaraman the link you posted went to https://github.com/gomods/athens/pull/1113/files#r263734261. Would you be able to move the prometheus.yml file to a new assets/ folder at the top level?

Also, regarding your question here: https://github.com/gomods/athens/pull/1113/files#r274158980, I don't think you need to monitor the teste2e and testunit containers, so it is ok if they get destroyed after they run. How does that sound?

@arschles
Copy link
Member

arschles commented Aug 5, 2019

@vaibhavjayaraman can you let us know if you're still working on this PR? Absolutely no problem if you're not - I can pick it up. I just wanted to check in.

@vaibhavjayaraman
Copy link
Author

Sorry about my absence. I've been busy lately and don't know when I'll be able to finish this so if it is pressing please feel free to take over.

@arschles
Copy link
Member

Not a problem @vaibhavjayaraman! AFAIK this isn't pressing for other folks. I'll leave this open for now, and if you get a chance to come back to it, we'll be here. Cheers!

@arschles arschles changed the base branch from master to main June 15, 2020 19:21
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.

Add Prometheus service to Athens docker-compose
5 participants