-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Including list of common units for formatting purposes in Backend. #19138
Conversation
What's the use case for storing the units in the database? There aren't that many units for data values. Why not just hardcode them in the UI code? |
We would like to avoid the necessity to change the code if a new unit is needed. We hope it can be just added to DB and the rest will work transparently. We also consider storing field-unit relation, especially for GL own fields, but probably for all other fields as well. |
7ef458d
to
039bf30
Compare
Hmm, can't we check which units would be needed and ship a pre-defined list? There aren't that many different common units. That would avoid the database collection, API endpoint, and the code for all of that.
Okay, got it. How about pre-define the units (enum?) and then use it for the field-unit relation. For the UI we could use our existing infrastructure to generate TypeScript files for the pre-defined units so can maintain them in a single place. What do you think? |
We can, but then we end up with a close list of units. With this approach if someone needs a new set of units, it is easy to expand and upgrade. We can add migrations to add new unit sets, throw some new units manually into DB if rich client pays for this, or even create a UI for importing/creating additional unit at some point in the future, if needed.
Is it really problematic that we have a new collection? It won't be big, so I assume it is not storage that you are worried about, but the existence of the collection itself?
You mean that somewhere in the DB we would refer to FE-only enum values? |
@bernd - FYI, we are supposed to provide around 13 units initially. |
I doubt that we need more than this: 😄 |
Heh, it seems they don't even support auto-scaling yet, at least not in this file. ;) |
@bernd - in order to proceed with the topic, I could also introduce a second implementation of the unit service, in memory one, and work on that in the near future. It would be faster than our current discussion and we will be able to switch to the db service if I managed to persuade you that we need it at some point. What do you think? :) |
Sorry for the sluggish responses. I was at an off-site meeting.
The additional database collection is not the issue. To me, making a finite and well-known collection of units configurable via the database and having an API for them introduces unneeded complexity. I think there is a limited number of units that make sense in a Graylog context. It's also easy to add more in a new release when customers request them. (also easier to backport when we only have to maintain a static list) Until now we don't have any units, so it's probably okay for customers to wait for the next release to get more exotic ones. 😄
What I had in mind was to define the units in a resource file. For the frontend, we could generate TypeScript code from that resource file at build time. That way, we don't need an API endpoint and HTTP requests to get the available units. Similar to how we generate TypeScript code for HTTP API models.
If you and your team think we need the ability to make units configurable via the database and it's worth the added complexity, you should continue with your current approach. |
@luk-kaminski If you continue with the database approach, please remember to implement content-pack support for units and add dependency resolution for the view entities. Otherwise, the referenced units in views might be broken. |
We are not sure about the decision yet. Will hopefully have a meeting tomorrow. |
@luk-kaminski, @bernd: I would say we should move these definitions to the code. Previously, I was torn between the complexity of the implementation and the fact that it is already implemented, but the fact that we would still need to add content pack support pushes me over the edge towards a simpler solution. |
a93faa7
to
e44df82
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round of feedback, as this will probably require some changes that affect the rest of the review.
graylog2-server/src/main/java/org/graylog/plugins/views/search/rest/MappedFieldTypeDTO.java
Outdated
Show resolved
Hide resolved
...rver/src/main/java/org/graylog/plugins/formatting/units/provider/SupportedUnitsProvider.java
Outdated
Show resolved
Hide resolved
Removed REST endpoints and further remainings of Mongo code. Added JSON file as a source of suported units. Copying units json for FE in mvn compile phase Folder for common resources. Data moved from there to BE resources, by Maven With units hardcoded in file, some functionality does not have sense anymore and was removed Units are referred by identifiers List of fields with default units prepared
d936ce2
to
c390cc7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better. Added some more inline comments for improvements.
...log2-server/src/main/java/org/graylog/plugins/formatting/units/fields/FieldUnitObtainer.java
Show resolved
Hide resolved
...rver/src/main/java/org/graylog/plugins/formatting/units/provider/SupportedUnitsProvider.java
Outdated
Show resolved
Hide resolved
graylog2-server/src/main/java/org/graylog2/indexer/fieldtypes/MappedFieldTypesServiceImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
Hardcoded list of supported units.
Fields can have unit associated with them.
Some basic GL fields get default/hardcoded unit association.
/nocl
Fixes #19157
Fixes #19299
Motivation and Context
How Has This Been Tested?
Manually and with new unit tests.
Types of changes
Checklist: