Skip to content

Add datasource and taskId to metrics emitted by peons #1967

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

Merged
himanshug merged 1 commit into from
Dec 3, 2015
Merged

Add datasource and taskId to metrics emitted by peons #1967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions indexing-service/src/main/java/io/druid/indexing/overlord/ForkingTaskRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@
import io.druid.indexing.common.tasklogs.LogUtils;
import io.druid.indexing.overlord.config.ForkingTaskRunnerConfig;
import io.druid.indexing.worker.config.WorkerConfig;
import io.druid.query.DruidMetrics;
import io.druid.server.DruidNode;
import io.druid.server.metrics.MonitorsConfig;
import io.druid.tasklogs.TaskLogPusher;
import io.druid.tasklogs.TaskLogStreamer;
import org.apache.commons.io.FileUtils;
Expand Down Expand Up @@ -297,6 +299,24 @@ public TaskStatus call()
}
}

// Add dataSource and taskId for metrics
command.add(
String.format(
"-D%s%s=%s",
Copy link
Copy Markdown
Member

@xvrl xvrl Nov 13, 2015

Choose a reason for hiding this comment

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

do we need to do any kind of escaping when interpolating the strings?

Copy link
Copy Markdown
Member Author

@nishantmonu51 nishantmonu51 Nov 13, 2015

Choose a reason for hiding this comment

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

I tested this with having blank spaces and quotes inside datasource name and its working fine.
don't think we need to do escaping there.

MonitorsConfig.METRIC_DIMENSION_PREFIX,
DruidMetrics.DATASOURCE,
task.getDataSource()
)
);
command.add(
String.format(
"-D%s%s=%s",
MonitorsConfig.METRIC_DIMENSION_PREFIX,
DruidMetrics.TASK_ID,
task.getId()
)
);

command.add(String.format("-Ddruid.host=%s", childHost));
command.add(String.format("-Ddruid.port=%d", childPort));

Expand Down
1 change: 1 addition & 0 deletions processing/src/main/java/io/druid/query/DruidMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public class DruidMetrics
public final static String TYPE = "type";
public final static String INTERVAL = "interval";
public final static String ID = "id";
public final static String TASK_ID = "taskId";
public final static String STATUS = "status";

// task metrics
Expand Down
42 changes: 42 additions & 0 deletions server/src/main/java/io/druid/server/metrics/MetricsModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,21 @@
import com.google.inject.name.Names;
import com.metamx.common.logger.Logger;
import com.metamx.emitter.service.ServiceEmitter;
import com.metamx.metrics.JvmCpuMonitor;
import com.metamx.metrics.JvmMonitor;
import com.metamx.metrics.Monitor;
import com.metamx.metrics.MonitorScheduler;
import com.metamx.metrics.SysMonitor;
import io.druid.concurrent.Execs;
import io.druid.guice.DruidBinders;
import io.druid.guice.JsonConfigProvider;
import io.druid.guice.LazySingleton;
import io.druid.guice.ManageLifecycle;

import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Properties;
import java.util.Set;

/**
Expand Down Expand Up @@ -95,4 +101,40 @@ public MonitorScheduler getMonitorScheduler(
monitors
);
}

@Provides
@ManageLifecycle
public JvmMonitor getJvmMonitor(Properties props)
{
return new JvmMonitor(getDimensions(props));
}

@Provides
@ManageLifecycle
public JvmCpuMonitor getJvmCpuMonitor(Properties props)
{
return new JvmCpuMonitor(getDimensions(props));
}

@Provides
@ManageLifecycle
public SysMonitor getSysMonitor(Properties props)
{
return new SysMonitor(getDimensions(props));
}
Copy link
Copy Markdown
Contributor

@himanshug himanshug Nov 17, 2015

Choose a reason for hiding this comment

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

is dataSource and taskId relevant for SysMonitor? Wouldn't it report same thing independent of the jvm.

Copy link
Copy Markdown
Member Author

@nishantmonu51 nishantmonu51 Nov 18, 2015

Choose a reason for hiding this comment

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

yeah, it reports the system level metrics and will be same for all the peons, I think it will still be useful to have a way to filter system level metrics for a given task in order to trace/debug issues better. e.g If one of the tasks is not able to ingest properly, it might be useful to filter the network metrics on that node by just selecting the taskID or dataSource.

Copy link
Copy Markdown
Contributor

@himanshug himanshug Nov 18, 2015

Choose a reason for hiding this comment

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

ok


private Map<String, String[]> getDimensions(Properties props)
{
Map<String, String[]> dimensions = new HashMap<>();
for (String property : props.stringPropertyNames()) {
if (property.startsWith(MonitorsConfig.METRIC_DIMENSION_PREFIX)) {
dimensions.put(
property.substring(MonitorsConfig.METRIC_DIMENSION_PREFIX.length()),
new String[]{props.getProperty(property)}
Copy link
Copy Markdown
Contributor

@drcrallen drcrallen Nov 16, 2015

Choose a reason for hiding this comment

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

Have you tested this with the LoggingEmitter? and null property value?

Copy link
Copy Markdown
Member Author

@nishantmonu51 nishantmonu51 Nov 18, 2015

Choose a reason for hiding this comment

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

I verified it with loggingEmitter,
the key we got is from an existing property, afaik in this case the value cannot be null here, am i missing anything ?

Copy link
Copy Markdown
Contributor

@drcrallen drcrallen Nov 18, 2015

Choose a reason for hiding this comment

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

I might be missing it.

Where guarantees that the property exists?

Copy link
Copy Markdown
Contributor

@drcrallen drcrallen Nov 18, 2015

Choose a reason for hiding this comment

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

oh nevermind

);
}
}
return dimensions;
}

}
2 changes: 2 additions & 0 deletions server/src/main/java/io/druid/server/metrics/MonitorsConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
*/
public class MonitorsConfig
{
public final static String METRIC_DIMENSION_PREFIX = "druid.metrics.emitter.dimension.";

@JsonProperty("monitors")
@NotNull
private List<Class<? extends Monitor>> monitors = Lists.newArrayList();
Expand Down