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

Refactor the complete code #49

Conversation

rustycl0ck
Copy link
Member

@rustycl0ck rustycl0ck commented Aug 4, 2020

  • Make the working of this exporter similar to that of the blackbox_exporter to allow probing multiple targets.
  • Add functionality to add headers to the request
  • Update the example config to use headers as well as the metrics keys in alignment with the new code
  • Add default header 'Accept: application/json'

Closes #43 #30 #14

@rustycl0ck rustycl0ck force-pushed the feature/scrape-multiple-endpoints branch from 915489f to 4ea7ef2 Compare August 4, 2020 06:19
* Make the working of this exporter similar to that of the blackbox_exporter to allow probing multiple targets.
* Add functionality to add headers to the request
* Update the example config to use `headers` as well as the `metrics` keys in alignment with the new code
* Add default header 'Accept: application/json'

Signed-off-by: rustyclock <[email protected]>
@rustycl0ck rustycl0ck force-pushed the feature/scrape-multiple-endpoints branch from 4ea7ef2 to c869516 Compare August 4, 2020 06:21
@SuperQ
Copy link
Contributor

SuperQ commented Aug 4, 2020

It would be useful to add a Prometheus config example with the required relabeling to the README.

Signed-off-by: rustyclock <[email protected]>
Signed-off-by: rustyclock <[email protected]>
@rustycl0ck
Copy link
Member Author

Migrated to promlog, but don't know if I should have used a global logger variable in the package, instead of passing it around to each function. Will wait for review/feedback.

@SuperQ
Copy link
Contributor

SuperQ commented Aug 5, 2020

Good question, I'm not very opinionated on how the logger is supposed to be used.

Maybe @roidelapluie has some opinions?

@SuperQ SuperQ requested review from roidelapluie and SuperQ August 5, 2020 14:49
@roidelapluie
Copy link
Contributor

Global - no, but you could create a struct that implements Collector and attach the functions to it - and use it from there

Signed-off-by: rustyclock <[email protected]>
@rustycl0ck
Copy link
Member Author

Switched to kingpin.

Closes #50 #28

cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
@rustycl0ck
Copy link
Member Author

Closes #33

@rustycl0ck rustycl0ck force-pushed the feature/scrape-multiple-endpoints branch from cfb0e53 to a6b9654 Compare August 19, 2020 02:59
Signed-off-by: rustyclock <[email protected]>
@rustycl0ck
Copy link
Member Author

rustycl0ck commented Aug 20, 2020

Addresses #44 also, since I added some unit tests now.
I don't think I'll be making anymore changes now (unless I find any bugs or you guys request any modifications) except creating a helm chart for deployment (which can/will probably be a separate PR).

Copy link
Contributor

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

LGTM

@SuperQ
Copy link
Contributor

SuperQ commented Sep 23, 2020

Ping @roidelapluie, would you mind reviewing this?

@roidelapluie
Copy link
Contributor

Can we use another package name that internal as it prevents reuse of the code elsewhere?

Copy link
Contributor

@roidelapluie roidelapluie left a comment

Choose a reason for hiding this comment

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

Some comments :) Awesome work

internal/util.go Outdated Show resolved Hide resolved
internal/util.go Outdated Show resolved Hide resolved
internal/util.go Outdated Show resolved Hide resolved
relabel_configs:
- source_labels: [__param_target]
target_label: endpoint
action: replace
Copy link
Contributor

Choose a reason for hiding this comment

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

An exemples à la blackbox_exporter might be better? With the json as target

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I didn't understand this fully I think.
Current example set up:
prometheus -> scrapes json_exporter -> fetches localhost:8000/examples/data.json
Proposed example set up:
prometheus -> scrapes blackbox_exporter -> probes json_exporter? -> fetches localhost:8000/examples/data.json?
🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's more about how you use Prometheus configs.

...
    static_configs:
      - targets:
        - http://host-1:8000/examples/data.json
        - http://host-2:8000/other-examples/data.json
    relabel_configs:
      - source_labels: [__address__]
        target_label: __param_target
      - source_labels: [__param_target]
        target_label: instance
      - target_label: __address__
        replacement: 127.0.0.1:7979  # The json exporter's real hostname:port.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the clarification, I have updated the example prometheus config. Let me know if the updated one looks good.

config/config.go Outdated Show resolved Hide resolved
cmd/main_test.go Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
@rustycl0ck
Copy link
Member Author

Thanks for the awesome review @roidelapluie. I have made the requested changes and addressed the remaining.

examples/prometheus.yml Outdated Show resolved Hide resolved
examples/prometheus.yml Outdated Show resolved Hide resolved
examples/prometheus.yml Outdated Show resolved Hide resolved
@rustycl0ck
Copy link
Member Author

Ping @SuperQ @roidelapluie Can this be merged if all looks good now?

@roidelapluie roidelapluie merged commit f76b12d into prometheus-community:master Oct 28, 2020
@roidelapluie
Copy link
Contributor

Thanks!!

@SuperQ
Copy link
Contributor

SuperQ commented Oct 28, 2020

@rustycl0ck Anything else we should do before we cut a new release?

@rustycl0ck
Copy link
Member Author

@SuperQ Nothing more yet. Thanks 👍
Although, I have a use case where I need to provide different bearerToken for authentication to different servers.But I'll create a new issue for discussing the right way to handle that.

@rustycl0ck rustycl0ck deleted the feature/scrape-multiple-endpoints branch October 29, 2020 03:14
kouk added a commit to kouk/json_exporter that referenced this pull request Nov 2, 2020
After prometheus-community#49 was merged to master the build failed due to a linting error:

    GO111MODULE=on /go/bin/golangci-lint run  ./...
    exporter/util.go:162:10: Error return value of `io.Copy` is not checked
    (errcheck)
                    io.Copy(ioutil.Discard, resp.Body)
                           ^
    make: *** [Makefile.common:192: common-lint] Error 1

build failure here:
https://app.circleci.com/pipelines/github/prometheus-community/json_exporter/28/workflows/e2c53db3-d3a3-4faa-a5a6-3b1fbb5754a0/jobs/71

Signed-off-by: Konstantinos Koukopoulos <[email protected]>
@kouk kouk mentioned this pull request Nov 2, 2020
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.

3 participants