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

Add ATM dev environment docker-compose and API doc #3171

Merged
merged 8 commits into from
Aug 4, 2021

Conversation

albertteoh
Copy link
Contributor

@albertteoh albertteoh commented Aug 1, 2021

Signed-off-by: albertteoh [email protected]

Which problem is this PR solving?

Resolves #3107

Short description of the changes

  • Adds a docker-compose file with instructions on how to bring up a development/demo environment for the metrics query feature.
  • Additionally documents the metrics query API and available query parameters with example curl commands.
  • The plan is to refer to this in the Getting Started documentation.

Signed-off-by: albertteoh <[email protected]>
@albertteoh albertteoh requested a review from a team as a code owner August 1, 2021 06:10
@albertteoh albertteoh requested a review from joe-elliott August 1, 2021 06:10
@codecov
Copy link

codecov bot commented Aug 1, 2021

Codecov Report

Merging #3171 (297d490) into master (2722ea8) will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3171      +/-   ##
==========================================
+ Coverage   95.89%   95.95%   +0.05%     
==========================================
  Files         239      239              
  Lines       14649    14649              
==========================================
+ Hits        14048    14056       +8     
+ Misses        523      517       -6     
+ Partials       78       76       -2     
Impacted Files Coverage Δ
...lugin/sampling/strategystore/adaptive/processor.go 100.00% <0.00%> (+0.64%) ⬆️
cmd/query/app/server.go 95.58% <0.00%> (+1.47%) ⬆️
cmd/query/app/static_handler.go 97.60% <0.00%> (+1.79%) ⬆️

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 2722ea8...297d490. Read the comment docs.

@@ -0,0 +1,173 @@
# atm-dev
Development environment for ATM
Copy link
Member

Choose a reason for hiding this comment

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

what is ATM? Please spell out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for picking up on this, I've elaborated a little more on what ATM stands for, the architecture of the development/demo environment and explanation of each component with relevant links, and an illustration of how the components interact with one another.

albertteoh and others added 2 commits August 1, 2021 17:50
Signed-off-by: albertteoh <[email protected]>
albertteoh and others added 2 commits August 1, 2021 17:59
Signed-off-by: albertteoh <[email protected]>
Signed-off-by: albertteoh <[email protected]>
curl http://localhost:16686/api/metrics/minstep | jq .
```

# HTTP API
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm planning to add this section into https://www.jaegertracing.io/docs/latest/apis/, would that be an appropriate place or is it sufficient to leave this API doc here?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be there as well, IMO.

docker-compose/monitor/README.md Outdated Show resolved Hide resolved
docker-compose/monitor/README.md Outdated Show resolved Hide resolved
curl http://localhost:16686/api/metrics/minstep | jq .
```

# HTTP API
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be there as well, IMO.

jaeger:
networks:
- backend
image: jaegertracing/all-in-one:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file being tested via CI? If not, it might be better to use a fixed version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this file being tested via CI?

Nope.

it might be better to use a fixed version.

I'd like to consider one hypothetical scenario, keeping in mind that the ATM/Monitor feature is still under active development (the UI isn't ready yet).

We set the jaeger all-in-one tag to v1.24 here. If someone makes a change now and merges to master before v1.25 is released, if I understand correctly, will users need to wait until v1.26 to obtain/use the features just merged?

otel_collector:
networks:
- backend
image: otel/opentelemetry-collector-contrib:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

docker-compose/monitor/docker-compose.yml Show resolved Hide resolved
Signed-off-by: albertteoh <[email protected]>
@albertteoh albertteoh merged commit 64aeccb into jaegertracing:master Aug 4, 2021
@albertteoh albertteoh deleted the atm-dev branch August 4, 2021 11:03
@albertteoh
Copy link
Contributor Author

Thanks for the review Yuri/Juca!

@vprithvi vprithvi added this to the Release 1.25.0 milestone Aug 5, 2021
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.

Metrics Query Development Environment
4 participants