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.

Sign up for GitHub

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

Jump to bottom

[Heartbeat] Use service.name not service_name in configs #20330

Merged
merged 15 commits into from
Nov 12, 2020
Merged

[Heartbeat] Use service.name not service_name in configs #20330

merged 15 commits into from
Nov 12, 2020

Conversation

andrewvc
Copy link
Contributor

Using the nesting this way lets us handle future fields like
service.environment more cleanly if/when they become available.

See https://github.com/elastic/ecs/blob/master/rfcs/text/0002-rfc-environment.md
for the proposal to add service.environment

This is a follow-up to #19932 which has not yet been released, so this is not a breaking change.

CC @cyrille-leclerc who pointed out that this would be better.

@andrewvc
[Heartbeat] Use service.name not service_name in configs
1e43b22 
Using the nesting this way lets us handle future fields like
`service.environment` more cleanly if/when they become available.

See https://github.com/elastic/ecs/blob/master/rfcs/text/0002-rfc-environment.md
for the proposal to add service.environment
@andrewvc andrewvc added Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team release-note:skip The PR should be ignored when processing the changelog labels Jul 30, 2020
@andrewvc andrewvc requested a review from blakerouse July 30, 2020 02:06
@andrewvc andrewvc requested a review from a team as a code owner July 30, 2020 02:06
@andrewvc andrewvc self-assigned this Jul 30, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/uptime (Team:Uptime)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jul 30, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jul 30, 2020 •
edited by jenkins-beats-ci bot
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #20330 updated]

  • Start Time: 2020-11-12T19:15:47.782+0000

  • Duration: 23 min 36 sec

Test stats 🧪

Test Results
Failed 0
Passed 923
Skipped 13
Total 936

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ingest-management (Team:Ingest Management)

@ph
Copy link
Contributor

ph commented Oct 7, 2020

@andrewvc I don't think this is linked to Fleet so i've removed the team label, I've looked at the PR I think there are some debugger traces that you left in that PR.

@cyrille-leclerc
Copy link
Contributor

cyrille-leclerc commented Nov 3, 2020 •
edited
Loading

Hello @andrewvc,
Context: I am working on the standardisation of service.environment (ECS - rfc-environment.md )

  • Do you think this change can make the 7.10 release?
  • Does it make sense to later extend the service.* namespace of monitor definitions adding an environment field with a syntax that could look like service.environment. Example:
# EXPERIMENT - NOT SUPPORTED CONFIGURATION SNIPPET
heartbeat.monitors:
- type: http
  id: www-frontend-monitor
  name: Website Frontend Monitor
  service:
    name: www-frontend
    environment: production
  urls: ["https://www.my-ecommerce-company.com/status"]
  schedule: '@every 10s'
  check.response.status: 200
  timeout: 2s
- type: http
  id: www-frontend-monitor-staging
  name: Website Frontend Monitor - Staging
  service:
    name: www-frontend
    environment: staging
  urls: ["https://www.staging.my-ecommerce-company.com/status"]
  schedule: '@every 10s'
  check.response.status: 200
  timeout: 5s

@andrewvc andrewvc mentioned this pull request Nov 10, 2020
Open
@andrewvc
Copy link
Contributor Author

@cyrille-leclerc apologies, this fell through the cracks. It won't make 7.10.0 but it can make 7.10.1. I 100% agree we should also add an environment field (that would go in 7.11.0)

I've created a separate issue for the env stuff here: elastic/uptime#272

@cyrille-leclerc
Copy link
Contributor

Many thanks @andrewvc

@andrewvc
Copy link
Contributor Author

I've updated this PR to handle both service_name and service.name. Only service.name will be documented however

@andrewvc andrewvc removed the release-note:skip The PR should be ignored when processing the changelog label Nov 12, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented Nov 12, 2020 •
edited by jenkins-beats-ci bot
Loading

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 923
Skipped 13
Total 936

blakerouse
blakerouse approved these changes Nov 12, 2020
View reviewed changes
Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

Looks good, like the fallback to old setting.

@andrewvc andrewvc merged commit 425271b into elastic:master Nov 12, 2020
@andrewvc andrewvc deleted the apm-dotted-service branch November 12, 2020 20:21
andrewvc added a commit to andrewvc/beats that referenced this pull request Nov 12, 2020
@andrewvc
[Heartbeat] Use service.name not service_name in configs (elastic#20330)
25c564a 
Using the nesting this way lets us handle future fields like
service.environment more cleanly if/when they become available.

See https://github.com/elastic/ecs/blob/master/rfcs/text/0002-rfc-environment.md
for the proposal to add service.environment

This is a follow-up to elastic#19932 which has not yet been released, so this is not a breaking change.

(cherry picked from commit 425271b)
@andrewvc andrewvc mentioned this pull request Nov 12, 2020
Merged
andrewvc added a commit that referenced this pull request Nov 12, 2020
@andrewvc
[Heartbeat] Use service.name not service_name in configs (#20330) (#2…
166761e 
…2574)

Using the nesting this way lets us handle future fields like
service.environment more cleanly if/when they become available.

See https://github.com/elastic/ecs/blob/master/rfcs/text/0002-rfc-environment.md
for the proposal to add service.environment

This is a follow-up to #19932 which has not yet been released, so this is not a breaking change.

(cherry picked from commit 425271b)
@cyrille-leclerc
Copy link
Contributor

Thanks @andrewvc

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

@blakerouse blakerouse blakerouse approved these changes

Assignees

@andrewvc andrewvc

Labels
Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team v7.11.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@andrewvc @elasticmachine @ph @cyrille-leclerc @blakerouse