Skip to content

Consider changing the output of /application/env/{propertyName} #10178

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

Closed
snicoll opened this issue Sep 6, 2017 · 4 comments
Closed

Consider changing the output of /application/env/{propertyName} #10178

snicoll opened this issue Sep 6, 2017 · 4 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@snicoll
Copy link
Member

snicoll commented Sep 6, 2017

The endpoint that provides information about a single property looks like this now (for the JAVA_VERSION system property:

{
  "activeProfiles": [
    
  ],
  "propertySources": [
    {
      "name": "server.ports",
      "properties": {
        
      }
    },
    {
      "name": "servletContextInitParams",
      "properties": {
        
      }
    },
    {
      "name": "systemProperties",
      "properties": {
        "java.version": {
          "value": "1.8.0_121",
          "origin": null
        }
      }
    },
    {
      "name": "systemEnvironment",
      "properties": {
        
      }
    },
    {
      "name": "applicationConfig: [classpath:/application.properties]",
      "properties": {
        
      }
    }
  ]
}

I like the fact that the output is a bit more consistent with the main endpoint but:

  1. I'd like that we add a value root attribute with the actual value in the environment. Previously that's all we had and this information is lost. IMO, it's quite interesting to have the actual value without computing it from the first property source in the list
  2. I don't think that showing empty property sources are very interesting. I do think that showing in the output that this particular key has many source is super interesting but the fact it's not present in one is noise
@snicoll snicoll added for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged theme: actuator labels Sep 6, 2017
@wilkinsona
Copy link
Member

I'd like that we add a value root attribute with the actual value in the environment

This sounds like a good addition to me

I don't think that showing empty property sources are very interesting. I do think that showing in the output that this particular key has many source is super interesting but the fact it's not present in one is noise

If you're using the endpoint to check the value of a property and figure out why it has that value, seeing information about all of the property sources is useful. If property sources that do not contain the property are omitted you can't tell if that source was missing (e.g. your application.properties file wasn't picked up) or if you messed up configuring the property in that source. In short, I disagree that a property not being present in a property source is noise.

We could, perhaps, convey the same information more concisely. We could list all of the sources where the property was found, including the origin information. The sources that exist but don't contain the property could then be listed separately. This suggestion has two downsides:

  1. The format of the propertySources entry would differ from the main /application/env endpoint
  2. We'd lose some information about the ordering of all the sources

1 isn't so bad, but 2 makes the endpoint less useful as it no longer tells you which sources you could add a property to if you wanted to override the value of the property that's coming from a particular source.

In summary, I am in favour of adding the value of the specific property to the response in a separate entry in the map. I am not in favour of changing the property sources that are included in the response.

@snicoll
Copy link
Member Author

snicoll commented Sep 6, 2017

That makes sense to me, thanks.

@snicoll
Copy link
Member Author

snicoll commented Sep 6, 2017

We should also consider skipping the origin rather than adding it with null

@snicoll snicoll added type: enhancement A general enhancement and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Sep 12, 2017
@snicoll snicoll added this to the 2.0.0.M5 milestone Sep 12, 2017
@snicoll snicoll self-assigned this Sep 21, 2017
@snicoll
Copy link
Member Author

snicoll commented Sep 26, 2017

So we've settled for this

{
  "property": {
    "value": "1.8.0_121",
    "source": "systemProperties"
  },
  "activeProfiles": [
    
  ],
  "propertySources": [
    {
      "name": "server.ports",
      "property": {
        
      }
    },
    {
      "name": "servletContextInitParams",
      "property": {
        
      }
    },
    {
      "name": "systemProperties",
      "property": {
        "value": "1.8.0_121",
        "origin": null
      }
    },
    {
      "name": "systemEnvironment",
      "property": {
        
      }
    },
    {
      "name": "applicationConfig: [classpath:/application.properties]",
      "property": {
        
      }
    }
  ]
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants