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

Add stacked chart widget #1284

Merged
merged 6 commits into from Jul 3, 2015
Merged

Add stacked chart widget #1284

merged 6 commits into from Jul 3, 2015

Conversation

@edmundoa
Copy link
Member

@edmundoa edmundoa commented Jul 2, 2015

Add support for stacked charts on dashboards. They are a bit different than other widgets, as they need to have a list of series that will be drawn.

@edmundoa edmundoa added this to the 1.2.0 milestone Jul 2, 2015
@joschi joschi self-assigned this Jul 3, 2015
this.renderer = renderer;
this.interpolation = interpolation;

this.chartSeries = Lists.newArrayList();

This comment has been minimized.

@joschi

joschi Jul 3, 2015
Contributor

The list could be initialized with the correct size (series.size()).

config.put("renderer", renderer);
config.put("interpolation", interpolation);

List<Map<String, String>> series = Lists.newArrayList();

This comment has been minimized.

@joschi

joschi Jul 3, 2015
Contributor

The list could be initialized with the correct size (chartSeries.size()).

filter = "streams:" + streamId;
}

final List<Map> results = Lists.newArrayList();

This comment has been minimized.

@joschi

joschi Jul 3, 2015
Contributor

The list could be initialized with the correct size (chartSeries.size()).

} catch (Searches.FieldTypeException e) {
String msg = "Could not calculate [" + this.getClass().getCanonicalName() + "] widget <" + getId() + ">. Not a numeric field? The field was [" + series.field + "]";
LOG.error(msg, e);
throw new RuntimeException(msg);

This comment has been minimized.

@joschi

joschi Jul 3, 2015
Contributor

The cause should also be included in the thrown RuntimeException.

This comment has been minimized.

@edmundoa

edmundoa Jul 3, 2015
Author Member

Good catch, will also change it in other widgets.

super(type, id, description, cacheTime, dashboard, creatorUserId, query, timerange);

this.interval = interval;
if (streamId != null && !streamId.isEmpty()) {

This comment has been minimized.

@joschi

joschi Jul 3, 2015
Contributor

This could be written as if (!isNullOrEmpty(streamId)) (for consistency).

This comment has been minimized.

@joschi

joschi Jul 3, 2015
Contributor

Or even simpler:

this.streamId = emptyToNull(streamId);

This comment has been minimized.

@edmundoa

edmundoa Jul 3, 2015
Author Member

Didn't know about emptyToNull, looks much better like that 👍


@Override
public Map<String, Object> getConfig() {
Map<String, Object> config = Maps.newHashMap();

This comment has been minimized.

@joschi

joschi Jul 3, 2015
Contributor

This should be an ImmutableMap to prevent accidental modifications.

w.description,
w.cacheTime,
TimeRange.factory((Map<String, Object>) w.config.get("timerange")),
(w.config.containsKey("stream_id") ? (String) w.config.get("stream_id") : null),

This comment has been minimized.

@joschi

joschi Jul 3, 2015
Contributor

Calling containsKey() isn't necessary as get() will return null if the key doesn't exist in the map.

This comment has been minimized.

@edmundoa

edmundoa Jul 3, 2015
Author Member

Makes sense, will also replace how we get the stream_id to create other widget instances, as they all look like that.

c.put("stream_id", streamId);
Map<String, Object> config = Maps.newHashMap();
config.putAll(getTimerange().getQueryParams());
config.putAll(super.getConfig());

This comment has been minimized.

@joschi

joschi Jul 3, 2015
Contributor

Shouldn't this be the first addition to the map so that the query parameters and the local config can overwrite settings?

This comment has been minimized.

@edmundoa

edmundoa Jul 3, 2015
Author Member

I think there is no overlapping of configuration keys on widgets, but doesn't hurt to move it up. 👍

}

@Override
public Map<String, Object> getConfig() {
Map<String, Object> config = Maps.newHashMap();
config.putAll(getTimerange().getQueryParams());
config.putAll(super.getConfig());

This comment has been minimized.

@joschi

joschi Jul 3, 2015
Contributor

Shouldn't this be the first addition to the map so that the query parameters and the local config can overwrite settings?

public Map<String, Object> getConfig() {
Map<String, Object> config = Maps.newHashMap();
config.putAll(getTimerange().getQueryParams());
config.putAll(super.getConfig());

This comment has been minimized.

@joschi

joschi Jul 3, 2015
Contributor

Shouldn't this be the first addition to the map so that the query parameters and the local config can overwrite settings?

.put("valuetype", statisticalFunction)
.put("renderer", renderer)
.put("interpolation", interpolation)
.putAll(super.getPersistedConfig());

This comment has been minimized.

@joschi

joschi Jul 3, 2015
Contributor

Shouldn't this be the first addition to the map so that the query parameters and the local config can overwrite settings?

@@ -65,15 +66,11 @@ public FieldChartWidget(MetricRegistry metricRegistry, Searches searches, String
final ImmutableMap.Builder<String, Object> persistedConfig = ImmutableMap.<String, Object>builder()

This comment has been minimized.

@joschi

joschi Jul 3, 2015
Contributor

I'm not sure if using a ImmutableMap.Builder is a good idea here. It will fail on calling build() if there were any duplicate keys added to it.

Are duplicate keys possible in that context?

This comment has been minimized.

@edmundoa

edmundoa Jul 3, 2015
Author Member

At the moment it there are no duplicated keys in widgets, so I think it's better to leave it as it is and modify it if ever needed.

this.renderer = (String) config.get("renderer");
this.interpolation = (String) config.get("interpolation");

this.chartSeries = Lists.newArrayList();

This comment has been minimized.

@joschi

joschi Jul 3, 2015
Contributor

The list could be initialized with the correct size (chartSeries.size()).

persistedConfig.put("stream_id", streamId);
}
.put("timerange", timeRange.getPersistedConfig())
.putAll(super.getPersistedConfig());

This comment has been minimized.

@joschi

joschi Jul 3, 2015
Contributor

Shouldn't this be the first addition to the map so that the query parameters and the local config can overwrite settings?

edmundoa added 5 commits Jul 3, 2015
Move general configuration keys to the top of the configuration, so its
values could be overwritten if needed.
- Use the less verbose `emptyToNull`
- Return configuration in an ImmutableMap to prevent modifications
@edmundoa
Copy link
Member Author

@edmundoa edmundoa commented Jul 3, 2015

Took care of most of the raised points, I hope it looks better now 👍

@joschi
Copy link
Contributor

@joschi joschi commented Jul 3, 2015

LGTM. 👍

joschi added a commit that referenced this pull request Jul 3, 2015
Add stacked chart widget
@joschi joschi merged commit 8f22d7b into master Jul 3, 2015
2 checks passed
2 checks passed
ci Jenkins build graylog2-server-integration-pr 19 has succeeded
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@joschi joschi deleted the stacked-charts branch Jul 3, 2015
@joschi joschi added the feature label Jul 3, 2015
@joschi joschi removed the ready-for-review label Jul 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants