joshelser commented on a change in pull request #3762:
URL: https://github.com/apache/hbase/pull/3762#discussion_r742108746
##########
File path: conf/hbase-env.sh
##########
@@ -143,10 +143,32 @@
# export GREP="${GREP-grep}"
# export SED="${SED-sed}"
-# Uncomment to enable trace, you can change the options to use other exporters
such as jaeger or
-# zipkin. See
https://github.com/open-telemetry/opentelemetry-java-instrumentation on how to
+# Tracing
+# Uncomment some combination of these lines to enable tracing. You should
change the options to use
+# the exporters appropriate to your environment. See
+# https://github.com/open-telemetry/opentelemetry-java-instrumentation for
details on how to
# configure exporters and other components through system properties.
-# export HBASE_TRACE_OPTS="-Dotel.resource.attributes=service.name=HBase
-Dotel.traces.exporter=logging -Dotel.metrics.exporter=none"
+#
+# The presence HBASE_TRACE_OPTS indicates that tracing should be enabled, and
serves as site-wide
+# settings.
+# export HBASE_TRACE_OPTS="-Dotel.traces.exporter=none
-Dotel.metrics.exporter=none"
+#
+# Per-process configuration variables allow for fine-grained configuration
control.
+# export HBASE_SHELL_OPTS="${HBASE_SHELL_OPTS} ${HBASE_TRACE_OPTS}
-Dotel.resource.attributes=service.name=hbase-shell"
Review comment:
Is it intentional that a user has to update their hbase-env.sh to
automatically pass through HBASE_TRACE_OPTS to the relevant commands when they
want it?
Meaning, when the user wants to turn on tracing, is there harm in setting
`otel.resource.attributes=service.name=...` for all of these opts
automatically, but let the user set their HBASE_TRACE_OPTS as they deem
appropriate?
##########
File path: bin/hbase
##########
@@ -493,7 +493,9 @@ add_jdk11_deps_to_classpath() {
}
add_opentelemetry_agent() {
- if ! agent_jar=$(find lib/trace -type f -name "opentelemetry-javaagent-*"
2>/dev/null); then
+ if [ -e "${OPENTELEMETRY_JAVAAGENT_PATH}" ] ; then
+ agent_jar="${OPENTELEMETRY_JAVAAGENT_PATH}"
+ elif ! agent_jar=$(find "${HBASE_HOME}/lib/trace" -type f -name
"opentelemetry-javaagent-*" 2>/dev/null); then
Review comment:
Do you want `find -L`? I'm not sure if we have already resolved
`HBASE_HOME` to be the "real" path by the time this function is called.
##########
File path: conf/hbase-env.sh
##########
@@ -143,10 +143,32 @@
# export GREP="${GREP-grep}"
# export SED="${SED-sed}"
-# Uncomment to enable trace, you can change the options to use other exporters
such as jaeger or
-# zipkin. See
https://github.com/open-telemetry/opentelemetry-java-instrumentation on how to
+# Tracing
+# Uncomment some combination of these lines to enable tracing. You should
change the options to use
+# the exporters appropriate to your environment. See
+# https://github.com/open-telemetry/opentelemetry-java-instrumentation for
details on how to
# configure exporters and other components through system properties.
-# export HBASE_TRACE_OPTS="-Dotel.resource.attributes=service.name=HBase
-Dotel.traces.exporter=logging -Dotel.metrics.exporter=none"
+#
+# The presence HBASE_TRACE_OPTS indicates that tracing should be enabled, and
serves as site-wide
+# settings.
+# export HBASE_TRACE_OPTS="-Dotel.traces.exporter=none
-Dotel.metrics.exporter=none"
+#
+# Per-process configuration variables allow for fine-grained configuration
control.
+# export HBASE_SHELL_OPTS="${HBASE_SHELL_OPTS} ${HBASE_TRACE_OPTS}
-Dotel.resource.attributes=service.name=hbase-shell"
Review comment:
> the operator has likely configured the service.name for a region
server in cluster A differently than for a region serve cluster B. I think it's
better to not make assumptions for the operator -- this approach gives them
complete control, while also providing a reasonable default that available by
simply uncommenting a few extra lines.
Ah, that is a great point I hadn't considered. I was assuming that I would
want all RegionServers to always be named "hbase-regionserver". To be more
explicitly, my original line of thinking was that we could put additions into
hbase-config.sh (or similar "internal" shell script which is automatically
source by `bin/hbase`) which do..
```
export HBASE_TRACE_OPTS="-Dotel.traces.exporter=none
-Dotel.metrics.exporter=none"
export HBASE_JSHELL_OPTS="${HBASE_JSHELL_OPTS}
-Dotel.resource.attributes=service.name=hbase-jshell"
export HBASE_HBCK_OPTS="${HBASE_HBCK_OPTS}
-Dotel.resource.attributes=service.name=hbase-hbck"
export HBASE_MASTER_OPTS="${HBASE_MASTER_OPTS}
-Dotel.resource.attributes=service.name=hbase-master"
export HBASE_REGIONSERVER_OPTS="${HBASE_REGIONSERVER_OPTS}
-Dotel.resource.attributes=service.name=hbase-regionserver"
...
```
Which would simplify the user-provide configs to be (assuming I only care
about master/regionserver tracing)
```
export HBASE_TRACE_OPTS="-Dotel.traces.exporter=none
-Dotel.metrics.exporter=none"
export HBASE_MASTER_OPTS="${HBASE_MASTER_OPTS} ${HBASE_TRACE_OPTS}"
export HBASE_REGIONSERVER_OPTS="${HBASE_REGIONSERVER_OPTS}
${HBASE_TRACE_OPTS}"
```
That said, your counter-argument is excellent and leave users to "undo"
additions we automatically add to environment variables is way worse than
verbose hbase-env.sh additions. I fully agree with your point about not making
assumptions. Will resolve this as such :). Better to revisit this after getting
some "real feedback"
##########
File path: bin/hbase
##########
@@ -493,7 +493,9 @@ add_jdk11_deps_to_classpath() {
}
add_opentelemetry_agent() {
- if ! agent_jar=$(find lib/trace -type f -name "opentelemetry-javaagent-*"
2>/dev/null); then
+ if [ -e "${OPENTELEMETRY_JAVAAGENT_PATH}" ] ; then
+ agent_jar="${OPENTELEMETRY_JAVAAGENT_PATH}"
+ elif ! agent_jar=$(find "${HBASE_HOME}/lib/trace" -type f -name
"opentelemetry-javaagent-*" 2>/dev/null); then
Review comment:
Yeah, I would expect for normal environments, your existing command
would work just fine. I do know that Cloudera's current offering would break if
we haven't fully resolved $HBASE_HOME (using client-facing-thirdparty as an
analogy)
```
[root@cod--vy18bale6smv-worker2 ~]# find
/opt/cloudera/parcels/CDH/lib/hbase/lib/client-facing-thirdparty -type f -name
'slf4j*'
[root@cod--vy18bale6smv-worker2 ~]# find -L
/opt/cloudera/parcels/CDH/lib/hbase/lib/client-facing-thirdparty -type f -name
'slf4j*'
/opt/cloudera/parcels/CDH/lib/hbase/lib/client-facing-thirdparty/slf4j-log4j12-1.7.30.jar
/opt/cloudera/parcels/CDH/lib/hbase/lib/client-facing-thirdparty/slf4j-api-1.7.30.jar
```
It looks like hbase-config.sh only sets HBASE_HOME based on the value from
`dirname $0` (where $0 is bin/hbase). Probably not a big deal either way, but
you might save me a headache later with a `find -L` (and I believe it to be
zero-risk).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
