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

Move 'spring.insights.web' properties to 'spring.http' #14313

Closed
philwebb opened this issue Sep 5, 2018 · 3 comments
Closed

Move 'spring.insights.web' properties to 'spring.http' #14313

philwebb opened this issue Sep 5, 2018 · 3 comments
Assignees
Labels
type: task A general task
Milestone

Comments

@philwebb
Copy link
Member

philwebb commented Sep 5, 2018

I'm not sure if we should move the spring.insights.web.log-request-details property elsewhere. It feels like it could become a central dependency that lots of packages start to use.

@philwebb philwebb added this to the 2.1.x milestone Sep 5, 2018
@philwebb philwebb added type: task A general task for: team-attention An issue we'd like other members of the team to review labels Sep 5, 2018
@bclozel
Copy link
Member

bclozel commented Sep 5, 2018

We could split it back and have both spring.mvc.log-request-details and spring.webflux.log-request-details. We already have spring.mvc.log-resolved-exception so it's not unusual.

It's just duplicating things a bit but maybe at this time, we don't have enough visibility about such features and it's not worth creating a new namespace at that point.

@philwebb
Copy link
Member Author

philwebb commented Sep 5, 2018

I think I prefer the duplication at this point, but lets see if anyone else has an opinion.

@snicoll
Copy link
Member

snicoll commented Sep 6, 2018

It really depends what we're trying to build. At this stage it is very limited so I agree a single "web" property would be enough (I am really not keen to have a "webflux" and "webmvc" property for this).

The intention was that we may want to improve this support in the future so that non-web components would also be taken into account (or other tuning options). But that's still vague so +1 for nuking it as long as we have a single web-related property.

@philwebb philwebb self-assigned this Sep 11, 2018
@philwebb philwebb changed the title Consider renaming InsightsProperties Move 'spring.insights.web' properties to 'spring.http' Sep 11, 2018
@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Sep 11, 2018
@philwebb philwebb modified the milestones: 2.1.x, 2.1.0.M3 Sep 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

No branches or pull requests

3 participants